chore: update docs and refactor the nix code#1
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR consolidates terraform/shell helper functions into a new shared Changes
Possibly related PRs
🚥 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 |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ROADMAP.md`:
- Around line 32-33: The roadmap entry mentioning "If secretspec provides a
provider SDK, implement the `Provider` trait alongside the CLI to get structured
secret declarations (`secretspec.toml`) for free." is ambiguous about priority;
update ROADMAP.md by turning that sentence into a separate checklist item and
clearly label it as optional (e.g., "Optional: Implement secretspec `Provider`
trait to emit `secretspec.toml`"), or explicitly state it is required and remove
the conditional phrasing; reference the `Provider` trait, `secretspec` SDK,
`secretspec.toml`, and the CLI in the new line so readers can find the related
implementation targets.
- Line 55: Replace the redundant phrase "CLI interface" in the checklist item "[
] CLI interface: `omnix-age encrypt`, `omnix-age decrypt`, `omnix-age rekey`"
with either "CLI" or the full phrase "Command-line interface" (e.g., "[ ] CLI:
`omnix-age encrypt`, `omnix-age decrypt`, `omnix-age rekey`") to avoid repeating
"interface."
- Around line 35-46: The Mermaid diagram in ROADMAP.md currently enforces a
linear flow (A→B→C→D→E→F→G→H) but several tasks can run in parallel; update the
graph so B[age encrypt/decrypt core] points to both C[keys.nix role parsing] and
D[Host key decryption], have C and D feed into E[CLI: encrypt/decrypt/rekey],
and also add direct edges from B to F[NixOS module] and B to G[deploy-rs
activation], then make E, F, and G converge into H[Integrate into omnix flake];
keep node A[Scaffold Rust crate] → B as-is and adjust labels (E) to reflect CLI
scope so the diagram accurately represents parallelizable work.
- Around line 27-30: The roadmap text embeds a consumer-specific reference
"st0x.liquidity"; remove that name and rewrite the sentence to describe the
generic workflow pattern omnix must support (e.g., committing `.toml.age` files
to git, using a deploy-time decryptor like `rage` to place decrypted files under
`/run/agenix/` using the host key such as `/etc/ssh/ssh_host_ed25519_key`, and
services reading plaintext from tmpfs). Replace the concrete project name with
neutral phrasing like "current pattern" or "existing deployments" and keep the
exact workflow symbols (`.toml.age`, `rage`, `/run/agenix/`,
`/etc/ssh/ssh_host_ed25519_key`, tmpfs) so the requirement remains precise but
infrastructure-agnostic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 06d4a90e-7690-4ae0-8dfa-44963cdb135c
📒 Files selected for processing (1)
ROADMAP.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
*
⚙️ CodeRabbit configuration file
Focus on providing constructive criticism. Whenever you see a suboptimal approach, suggest more idiomatic or robust alternative(s). Flag potential footguns. Suggest FP alternatives to mutable/imperative code. Point out architectural flaws like leaky abstractions, tight coupling, wrong level of abstraction, poor type modeling, over-abstraction, unclear domain boundaries. Code should generally be organized based on business concerns rather than technical aspects - suggest improvements if you find violations. Point out gaps in test coverage but suggest tests that are not too coupled to the implementation and actually test domain invariants and business logic.
Files:
ROADMAP.md
**/*.md
⚙️ CodeRabbit configuration file
Focus on the contents of the docs and not on cosmetic things like markdown formatting. We use markdown files for various docs including but not limited to the north star system specification, SPEC.md, the plan for how to get there, ROADMAP.md, guidelines for AI contributors, AGENTS.md, project overview and instructions for human contributors, README.md. Think about the target audience of a document when deciding what comment to leave. For specifications and designs, suggest potential product, architectural, and UI/UX improvements. For plans, suggest changes that would make things more parallelizable and deliverable-focused. For instructions, suggest better rules and guidelines and point out missing instructions. In all cases, flag needless bloat, prefer clear concise writing, and consider the structure of the document and order of the sections
Files:
ROADMAP.md
🪛 LanguageTool
ROADMAP.md
[style] ~55-~55: This phrase is redundant (‘I’ stands for ‘interface’). Use simply “CLI”.
Context: ...s identity, same as ragenix today - [ ] CLI interface: omnix-age encrypt, `omnix-age decryp...
(ACRONYM_TAUTOLOGY)
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (1)
ROADMAP.md (1)
55-55:⚠️ Potential issue | 🟡 MinorFix redundant acronym: "CLI interface".
"CLI" already means "Command-Line Interface", so "CLI interface" is redundant.
✍️ Proposed fix
-- [ ] CLI interface: `omnix-age encrypt`, `omnix-age decrypt`, `omnix-age rekey` +- [ ] CLI: `omnix-age encrypt`, `omnix-age decrypt`, `omnix-age rekey`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ROADMAP.md` at line 55, Change the redundant phrase "CLI interface" to either "CLI" or "command-line interface" in the ROADMAP entry that lists the commands (`omnix-age encrypt`, `omnix-age decrypt`, `omnix-age rekey`); update the line "- [ ] CLI interface: `omnix-age encrypt`, `omnix-age decrypt`, `omnix-age rekey`" to remove the duplicated word so it reads e.g. "- [ ] CLI: `omnix-age encrypt`, `omnix-age decrypt`, `omnix-age rekey`".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flake.nix`:
- Around line 85-99: The dev shell currently hardcodes packages while deriving
shellHook from checks.git-hooks; instead derive the devShell toolchain from the
same check so the shell follows upstream
enabledPackages/extraHooks/rustToolchain logic — update devShells.default to
import the packages and shellHook from checks.git-hooks
(checks.git-hooks.lib.${system}.enabledPackages or equivalent) so the hook
runner and any extraHooks/rustToolchain are included automatically, keep using
shellHook from checks.git-hooks and remove the manual package list
(nixfmt/deadnix/taplo) so toolchain changes in lib/hooks.nix propagate to the
dev shell.
In `@lib/bootstrap.nix`:
- Around line 18-24: The runtimeInputs array is missing the openssh package used
by the wrapper (ssh is invoked later), so update the runtimeInputs declaration
to include pkgs.openssh (e.g., add pkgs.openssh alongside pkgs.rage, pkgs.jq,
pkgs.gnused) so writeShellApplication declares ssh as a runtime dependency; keep
the conditional ragenixPkg handling unchanged.
In `@lib/default.nix`:
- Line 11: The mkGitHooks call passes an unused pkgs parameter even though
hooks.nix only consumes extraHooks and rustToolchain; remove pkgs from the
mkGitHooks invocation and update the caller so it no longer passes { inherit
pkgs; }, or alternatively, if you want to keep pkgs, add explicit usage or a
comment in hooks.nix documenting why pkgs is accepted but unused; target
symbols: mkGitHooks (call site), hooks.nix (module using extraHooks and
rustToolchain), and ensure any callers stop passing pkgs to avoid misleading API
consumers.
In `@lib/hooks.nix`:
- Around line 1-5: The top-level argument set in lib/hooks.nix uses a catch-all
`...` which silently accepts and discards extra args; either remove the `...` or
explicitly accept/document expected extra args (for example add an explicit
`pkgs ? null` parameter) and update the file header comment to state whether
extra arguments are ignored or which ones (like `pkgs`) are supported; locate
the argument list containing `extraHooks ? { }`, `rustToolchain ? null` and the
trailing `...` and either remove `...` or replace it with named parameters and a
short comment explaining behavior.
In `@lib/shell.nix`:
- Around line 37-42: The resolveIp shell block currently uses "jq -r" to extract
droplet_ipv4 into host_ip which returns "null" with exit code 0 when the output
is missing; change the jq invocation in the resolveIp block to use jq -e -r so
jq exits non‑zero on null/missing values and the script fails fast, ensuring
host_ip is never set to an invalid value; update the host_ip extraction line
that references ${tfState} inside resolveIp (the lines surrounding
parseIdentity, decryptState, and host_ip) to use jq -e -r and optionally
propagate the error immediately (i.e., do not suppress the exit) so downstream
SSH attempts in lib/bootstrap.nix and lib/terraform.nix receive a valid IP or
abort.
- Around line 15-18: The decrypted Terraform files are written with
caller-controlled permissions; update decryptState and decryptVars to enforce
restrictive perms by setting a strict umask (e.g., umask 077) before writing or
by immediately changing mode to 600 after writing the plaintext (use tfState and
tfVars symbols to locate the writes). Ensure the umask change or chmod 600 is
applied only around the decrypt write operations so the rest of the script is
unaffected, and keep error handling intact if the permission change fails.
In `@lib/terraform.nix`:
- Around line 10-27: The buildInputs list in lib/terraform.nix is missing the
nix binary required at runtime by the inherited helpers encryptState and
encryptVars (from shell.nix) which call `nix eval`; update the buildInputs array
(the variable named buildInputs) to include pkgs.nix alongside pkgs.terraform,
pkgs.rage, and pkgs.jq so the runtimeInputs used by mkTask/writeShellApplication
provide the `nix` executable for tasks like tfApply, tfDestroy, tfImport and
rekey.
In `@modules/base.nix`:
- Line 63: Replace the imperative activation script at
system.activationScripts.per-service-profiles.text that runs mkdir -p
/nix/var/nix/profiles/per-service with a declarative systemd tmpfiles rule: add
or replace configuration using systemd.tmpfiles.rules (or
systemd.tmpfiles.rules."per-service-profiles") to declare the directory path,
ownership and mode (e.g. d /nix/var/nix/profiles/per-service 0755 root root -)
so the directory is created declaratively with correct permissions instead of
using an activation shell command.
In `@modules/services.nix`:
- Around line 45-70: Replace the verbose conditional merges that use the pattern
"// ( if ... then { ... } else { } )" with lib.optionalAttrs to cleanly include
attributes only when conditions hold: for the block that sets User/Group use
lib.optionalAttrs (cfg.group != null) // { User = cfg.user; Group = cfg.group;
}, for SupplementaryGroups use lib.optionalAttrs (cfg.dynamicUser && cfg.group
!= null) // { SupplementaryGroups = [ cfg.group ]; }, and for ReadWritePaths use
lib.optionalAttrs (svcCfg.dataDir != null) // { ReadWritePaths = [
svcCfg.dataDir ]; } so the attributes are included idiomatically and merged
without the explicit empty-else objects.
In `@modules/storage.nix`:
- Around line 10-13: The option volumeName currently accepts any string which
can produce invalid by-id device paths; add a validation to the lib.mkOption for
volumeName to restrict characters (e.g. only alphanumerics, dot, underscore,
hyphen) by adding a validate field such as validate = name: lib.match
"^[A-Za-z0-9._-]+$" name != null or similar, and provide a clear errorMessage
like "volumeName may only contain letters, digits, '.', '_', '-'"; keep type =
lib.types.str and reference the volumeName option and lib.mkOption in your
change.
In `@README.md`:
- Line 42: Add a short “Migration” note in README describing how to replace
usages of mkRemote with lib.mkGitHooks: state that mkRemote has been renamed to
lib.mkGitHooks, provide a minimal before/after snippet showing the old mkRemote
invocation and the new lib.mkGitHooks invocation (preserving hook list: nixfmt,
deadnix, taplo and optional rustfmt), and mention any argument/name changes or
defaults (e.g., rustfmt is optional) so users can quickly update their code.
In `@templates/do-service/flake.nix`:
- Around line 40-48: The deployConfig is duplicated with inconsistent package
references (top-level uses self.packages.x86_64-linux.default while the
per-system block uses pkgs.hello); fix by extracting the package reference into
a single variable (e.g., package = self.packages.${system}.default or a shared
let-bound pkg variable) and replace both occurrences of
self.packages.x86_64-linux.default and pkgs.hello to reference that single
variable so both the flake-level deployConfig and the per-system wrappers use
the exact same package reference (update deployConfig, the per-system section,
and any uses of pkgs.hello accordingly).
---
Duplicate comments:
In `@ROADMAP.md`:
- Line 55: Change the redundant phrase "CLI interface" to either "CLI" or
"command-line interface" in the ROADMAP entry that lists the commands
(`omnix-age encrypt`, `omnix-age decrypt`, `omnix-age rekey`); update the line
"- [ ] CLI interface: `omnix-age encrypt`, `omnix-age decrypt`, `omnix-age
rekey`" to remove the duplicated word so it reads e.g. "- [ ] CLI: `omnix-age
encrypt`, `omnix-age decrypt`, `omnix-age rekey`".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c264208c-bf58-46d4-91a4-b1e4b2fd453f
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
README.mdROADMAP.mdflake.nixlib/bootstrap.nixlib/default.nixlib/deploy.nixlib/hooks.nixlib/remote.nixlib/shell.nixlib/terraform.nixmodules/base.nixmodules/digitalocean.nixmodules/disko.nixmodules/firewall.nixmodules/services.nixmodules/storage.nixtemplates/do-service/config/secrets.nixtemplates/do-service/flake.nixtemplates/do-service/infra/secrets.nixtemplates/do-service/keys.nixtemplates/do-service/os.nix
💤 Files with no reviewable changes (1)
- lib/remote.nix
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
*
⚙️ CodeRabbit configuration file
Focus on providing constructive criticism. Whenever you see a suboptimal approach, suggest more idiomatic or robust alternative(s). Flag potential footguns. Suggest FP alternatives to mutable/imperative code. Point out architectural flaws like leaky abstractions, tight coupling, wrong level of abstraction, poor type modeling, over-abstraction, unclear domain boundaries. Code should generally be organized based on business concerns rather than technical aspects - suggest improvements if you find violations. Point out gaps in test coverage but suggest tests that are not too coupled to the implementation and actually test domain invariants and business logic.
Files:
README.mdflake.nixROADMAP.md
**/*.md
⚙️ CodeRabbit configuration file
Focus on the contents of the docs and not on cosmetic things like markdown formatting. We use markdown files for various docs including but not limited to the north star system specification, SPEC.md, the plan for how to get there, ROADMAP.md, guidelines for AI contributors, AGENTS.md, project overview and instructions for human contributors, README.md. Think about the target audience of a document when deciding what comment to leave. For specifications and designs, suggest potential product, architectural, and UI/UX improvements. For plans, suggest changes that would make things more parallelizable and deliverable-focused. For instructions, suggest better rules and guidelines and point out missing instructions. In all cases, flag needless bloat, prefer clear concise writing, and consider the structure of the document and order of the sections
Files:
README.mdROADMAP.md
🪛 LanguageTool
ROADMAP.md
[style] ~55-~55: This phrase is redundant (‘I’ stands for ‘interface’). Use simply “CLI”.
Context: ...s identity, same as ragenix today - [ ] CLI interface: omnix-age encrypt, `omnix-age decryp...
(ACRONYM_TAUTOLOGY)
🔇 Additional comments (18)
templates/do-service/config/secrets.nix (1)
1-4: Formatting-only refactor looks good.No functional change introduced; this is a clean readability improvement.
templates/do-service/infra/secrets.nix (1)
1-4: Formatting update is safe.Behavior and key bindings remain intact.
modules/firewall.nix (1)
3-6: No concerns in this segment.This is a readability-only refactor.
modules/base.nix (1)
1-11: Formatting refactor is clean and consistent.No functional regressions detected in these segments.
Also applies to: 46-49, 65-75
modules/disko.nix (1)
3-6: LGTM on this change.Pure formatting reflow; behavior is unchanged.
modules/storage.nix (1)
3-6: Formatting-only changes are fine.No functional delta in this block.
modules/digitalocean.nix (1)
1-11: Looks good.These edits are readability-oriented and preserve behavior.
Also applies to: 33-36, 49-54
README.md (1)
39-39: No changes needed. Theremote SSHfunctionality is actively provided bylib.mkTerraformvia theremotewrapper inlib/terraform.nix, and the README description is accurate.templates/do-service/os.nix (1)
3-6: LGTM!The multi-line formatting improves readability without changing semantics.
Also applies to: 31-39
templates/do-service/keys.nix (1)
3-5: LGTM!Formatting improvements to the role mappings. The placeholder keys are appropriately marked as examples.
Also applies to: 9-20
modules/services.nix (1)
8-26: LGTM!The list-based construction for
execStartwithbuiltins.concatStringsSepis a clean approach for handling optional extra arguments.lib/deploy.nix (1)
3-9: LGTM!The signature and structural reformatting improves readability. The deploy profile construction logic is sound.
Also applies to: 38-60
ROADMAP.md (1)
74-92: Well-structured integration test flow.The end-to-end lifecycle (provision → bootstrap → deploy → verify → redeploy → teardown) is clearly documented. The
mkIntegrationTestlib function is a good abstraction for consumer extensibility.lib/default.nix (1)
1-5: LGTM on the signature change.The spread
...allows forward compatibility. The removal ofnixpkgsfrom explicit parameters is appropriate since it's no longer used directly in lib functions.templates/do-service/flake.nix (1)
10-17: LGTM on the per-system restructuring.The use of
flake-utils.lib.eachDefaultSystemwith the//merge pattern properly separates system-independent outputs (nixosConfigurations, deploy) from per-system outputs (packages, devShells).Also applies to: 50-112
lib/hooks.nix (2)
30-31: LGTM!The
defaultHooks // extraHooksmerge order correctly allows consumers to override defaults with their custom hooks.
8-28: Verify hook structure matches git-hooks framework expectations.The hook definitions (e.g.,
nixfmt.enable = true,deadnix = { enable = true; excludes = [...]; }) assume a specific structure expected by thegit-hooksframework. Therustfmthook uses custom fields (entry,files,pass_filenames) which may or may not be recognized.What is the expected hook configuration structure for nix-community/git-hooks.nix?lib/bootstrap.nix (1)
61-69: Good guard around rekeying.Requiring both
ragenixPkgandsecretsRulesavoids emitting a half-configuredragenixinvocation.
| checks.git-hooks = git-hooks.lib.${system}.run { | ||
| inherit hooks; | ||
| src = self; | ||
| }; | ||
|
|
||
| formatter = pkgs.nixfmt; | ||
|
|
||
| devShells.default = pkgs.mkShell { | ||
| inherit (self.checks.${system}.git-hooks) shellHook; | ||
| packages = [ | ||
| pkgs.nixfmt | ||
| pkgs.deadnix | ||
| pkgs.taplo | ||
| ]; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Derive the dev-shell toolchain from the hook check.
Right now shellHook comes from checks.git-hooks, but the tool list is hand-maintained. That will drift from lib/hooks.nix:1-31 as soon as extraHooks or rustToolchain is used, and it skips the upstream enabledPackages pattern that brings the hook runner into the dev shell as well. (github.com)
♻️ Suggested refactor
- devShells.default = pkgs.mkShell {
- inherit (self.checks.${system}.git-hooks) shellHook;
- packages = [
- pkgs.nixfmt
- pkgs.deadnix
- pkgs.taplo
- ];
- };
+ devShells.default =
+ let
+ inherit (self.checks.${system}.git-hooks) shellHook enabledPackages;
+ in
+ pkgs.mkShell {
+ inherit shellHook;
+ buildInputs = enabledPackages;
+ };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flake.nix` around lines 85 - 99, The dev shell currently hardcodes packages
while deriving shellHook from checks.git-hooks; instead derive the devShell
toolchain from the same check so the shell follows upstream
enabledPackages/extraHooks/rustToolchain logic — update devShells.default to
import the packages and shellHook from checks.git-hooks
(checks.git-hooks.lib.${system}.enabledPackages or equivalent) so the hook
runner and any extraHooks/rustToolchain are included automatically, keep using
shellHook from checks.git-hooks and remove the manual package list
(nixfmt/deadnix/taplo) so toolchain changes in lib/hooks.nix propagate to the
dev shell.
|
|
||
| system.activationScripts.per-service-profiles.text = | ||
| "mkdir -p /nix/var/nix/profiles/per-service"; | ||
| system.activationScripts.per-service-profiles.text = "mkdir -p /nix/var/nix/profiles/per-service"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer systemd.tmpfiles over activation shell for static directory creation.
Using an activation shell command here is more imperative than necessary. A tmpfiles rule is more idiomatic and keeps ownership/mode declarative.
♻️ Proposed refactor
- system.activationScripts.per-service-profiles.text = "mkdir -p /nix/var/nix/profiles/per-service";
+ systemd.tmpfiles.rules = [
+ "d /nix/var/nix/profiles/per-service 0755 root root -"
+ ];As per coding guidelines, *: "suggest more idiomatic or robust alternative(s)" and "flag potential footguns."
📝 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.
| system.activationScripts.per-service-profiles.text = "mkdir -p /nix/var/nix/profiles/per-service"; | |
| systemd.tmpfiles.rules = [ | |
| "d /nix/var/nix/profiles/per-service 0755 root root -" | |
| ]; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/base.nix` at line 63, Replace the imperative activation script at
system.activationScripts.per-service-profiles.text that runs mkdir -p
/nix/var/nix/profiles/per-service with a declarative systemd tmpfiles rule: add
or replace configuration using systemd.tmpfiles.rules (or
systemd.tmpfiles.rules."per-service-profiles") to declare the directory path,
ownership and mode (e.g. d /nix/var/nix/profiles/per-service 0755 root root -)
so the directory is created declaratively with correct permissions instead of
using an activation shell command.
| volumeName = lib.mkOption { | ||
| type = lib.types.str; | ||
| description = | ||
| "DigitalOcean volume name (used in /dev/disk/by-id/scsi-0DO_Volume_<name>)"; | ||
| description = "DigitalOcean volume name (used in /dev/disk/by-id/scsi-0DO_Volume_<name>)"; | ||
| }; |
There was a problem hiding this comment.
Constrain volumeName to safe characters to avoid invalid device paths.
volumeName is used in a by-id device path, but the option accepts any string. Inputs with spaces or path-like characters can break mounts.
🛡️ Proposed fix
volumeName = lib.mkOption {
- type = lib.types.str;
+ type = lib.types.strMatching "^[A-Za-z0-9._-]+$";
description = "DigitalOcean volume name (used in /dev/disk/by-id/scsi-0DO_Volume_<name>)";
};As per coding guidelines, *: "Flag potential footguns" and "suggest more idiomatic or robust alternative(s)."
📝 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.
| volumeName = lib.mkOption { | |
| type = lib.types.str; | |
| description = | |
| "DigitalOcean volume name (used in /dev/disk/by-id/scsi-0DO_Volume_<name>)"; | |
| description = "DigitalOcean volume name (used in /dev/disk/by-id/scsi-0DO_Volume_<name>)"; | |
| }; | |
| volumeName = lib.mkOption { | |
| type = lib.types.strMatching "^[A-Za-z0-9._-]+$"; | |
| description = "DigitalOcean volume name (used in /dev/disk/by-id/scsi-0DO_Volume_<name>)"; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/storage.nix` around lines 10 - 13, The option volumeName currently
accepts any string which can produce invalid by-id device paths; add a
validation to the lib.mkOption for volumeName to restrict characters (e.g. only
alphanumerics, dot, underscore, hyphen) by adding a validate field such as
validate = name: lib.match "^[A-Za-z0-9._-]+$" name != null or similar, and
provide a clear errorMessage like "volumeName may only contain letters, digits,
'.', '_', '-'"; keep type = lib.types.str and reference the volumeName option
and lib.mkOption in your change.
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/base.nix (1)
26-29:⚠️ Potential issue | 🟡 MinorRemoving the default for
stateVersionis a breaking change.Consumers relying on the previous default (
"24.11") will now receive an evaluation error sincestateVersionis a required option without a default. While the template (templates/do-service/os.nix) has been updated, any external users of this module will need to explicitly setomnix.base.stateVersion.Consider adding an assertion with a helpful error message to guide users:
💡 Suggested assertion
config = lib.mkIf cfg.enable { + assertions = [ + { + assertion = cfg.stateVersion != ""; + message = "omnix.base.stateVersion must be set explicitly (e.g., \"24.11\")"; + } + ]; + services.openssh = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/base.nix` around lines 26 - 29, The module removed the default for the option stateVersion, which breaks consumers that relied on "24.11"; restore a safe failure path by adding an assertion in modules/base.nix guarding stateVersion: if the option is unset/null/empty, assert with a clear error message that tells users to set omnix.base.stateVersion (or update to the new templates/do-service/os.nix) and optionally mention the previous default value ("24.11") as a hint; locate the stateVersion lib.mkOption declaration and add the assertion immediately after it so evaluation fails with the helpful message instead of an obscure error.
♻️ Duplicate comments (8)
README.md (1)
42-42: 🛠️ Refactor suggestion | 🟠 MajorAdd migration guidance for
mkRemote→mkGitHooksrename.Users of the previous
lib.mkRemoteAPI will need guidance on how to migrate. Consider adding a brief note (e.g., in a "Migration" or "Changelog" section) explaining thatmkRemotehas been renamed tomkGitHooksand any signature changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 42, Add a short "Migration" note to the README explaining that the old API function lib.mkRemote has been renamed to lib.mkGitHooks; state the exact rename (lib.mkRemote → lib.mkGitHooks), list any signature changes (parameter names, defaults, or removed/added options) and show the minimal migration steps users must take (rename calls and update renamed options). Place this note near the existing API table or in a new "Changelog / Migration" section so users upgrading can quickly find it and cross-reference the symbols mkRemote and mkGitHooks.flake.nix (1)
97-104: 🧹 Nitpick | 🔵 TrivialDev shell packages are manually maintained and may drift from hooks.
The
shellHookis derived fromchecks.git-hooks, but the package list (nixfmt,deadnix,taplo) is hardcoded. Iflib/hooks.nixis extended withextraHooksorrustToolchain, the dev shell won't include the corresponding tools. Consider usingenabledPackagesfrom the git-hooks check output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flake.nix` around lines 97 - 104, The dev shell currently hardcodes packages in devShells.default using pkgs.mkShell while the shellHook comes from self.checks.${system}.git-hooks, which can also emit enabledPackages; replace or augment the hardcoded package list (pkgs.nixfmt, pkgs.deadnix, pkgs.taplo) with the enabledPackages exported by self.checks.${system}.git-hooks (e.g., use enabledPackages from that check and merge it with any unavoidable extras) so that additions in lib/hooks.nix like extraHooks or rustToolchain automatically appear in the dev shell; update references in devShells.default to use self.checks.${system}.git-hooks.enabledPackages together with shellHook.templates/do-service/flake.nix (1)
35-43:⚠️ Potential issue | 🟡 MinorDuplicate
deployConfigwith inconsistent package references.Two
deployConfigdefinitions exist:
- Lines 37-41 (flake-level): uses
self.packages.x86_64-linux.default- Lines 60-64 (per-system): uses
pkgs.helloWhile both currently resolve to the same placeholder, they will drift when
pkgs.hellois replaced. Consider defining the package once and referencing it consistently.Also applies to: 60-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/do-service/flake.nix` around lines 35 - 43, The two deployConfig blocks duplicate package selection inconsistently: one uses self.packages.x86_64-linux.default and the other uses pkgs.hello; unify by extracting a single package binding (e.g., define a variable like deployPackage or package = self.packages.x86_64-linux.default) and reference that variable in both omnix.lib.mkDeploy calls (the deployConfig in the flake-level and the per-system deployConfig), ensuring both the package field passed into mkDeploy and any per-system package references use the same identifier.lib/hooks.nix (1)
1-5: 🧹 Nitpick | 🔵 TrivialThe
...catch-all silently discardspkgs.Per
flake.nix:83, callers pass{ inherit pkgs; }but this function ignores it via.... Either document this behavior or remove the catch-all if no extra arguments are expected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/hooks.nix` around lines 1 - 5, The function parameter list in lib/hooks.nix currently swallows caller-provided pkgs via the catch-all "..." which discards { pkgs } from callers; change the parameter list to explicitly accept pkgs (e.g., replace "..., " with "pkgs" or "pkgs ? null" depending on whether it should be required) and update any uses in the function to reference that pkgs symbol (keep extraHooks and rustToolchain as before); this ensures callers passing { inherit pkgs; } are not ignored and preserves explicit argument semantics.lib/bootstrap.nix (1)
18-24:⚠️ Potential issue | 🟠 MajorMissing
pkgs.opensshinruntimeInputs.The script invokes
sshon lines 36 and 46, butopensshis not declared inruntimeInputs.writeShellApplicationrequires all runtime tools to be explicitly listed.runtimeInputs = [ pkgs.rage pkgs.jq pkgs.gnused + pkgs.openssh nixos-anywhere.packages.${system}.default ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/bootstrap.nix` around lines 18 - 24, The runtimeInputs list is missing pkgs.openssh while the script calls ssh (via writeShellApplication), so add pkgs.openssh to the runtimeInputs array alongside pkgs.rage, pkgs.jq, and pkgs.gnused; ensure it is included unconditionally (or follow the same conditional pattern as ragenixPkg if intended) so ssh is available at runtime and the writeShellApplication sandbox lists it as a runtime dependency.lib/terraform.nix (1)
22-27:⚠️ Potential issue | 🟠 MajorMissing
pkgs.nixinbuildInputs.The inherited helpers
encryptStateandencryptVarsfromlib/shell.nixexecutenix evalat runtime (see shell.nix lines 22-24 and 32-34). SincemkTaskusesruntimeInputs = buildInputs, tasks liketfApply,tfDestroy,tfImport,tfRekey, andrekeywill fail when invoking encryption helpers.buildInputs = [ pkgs.terraform pkgs.rage pkgs.jq + pkgs.nix ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/terraform.nix` around lines 22 - 27, The buildInputs list in lib/terraform.nix is missing pkgs.nix which causes the runtime helpers encryptState and encryptVars (from lib/shell.nix) to fail when they run nix eval; update the buildInputs expression used by mkTask (runtimeInputs = buildInputs) to include pkgs.nix alongside pkgs.terraform, pkgs.rage, and pkgs.jq, while preserving the conditional inclusion of ragenixPkg so tfApply/tfDestroy/tfImport/tfRekey/rekey can invoke the encryption helpers successfully.lib/shell.nix (2)
37-43:⚠️ Potential issue | 🟠 MajorFail fast when
droplet_ipv4is missing.
jq -routputs "null" (exit 0) if the path doesn't exist, allowing the script to continue with an invalid IP. Usejq -erto fail immediately. Also, the explicitrm -fon line 42 is redundant since the EXIT trap already handles cleanup.🔧 Proposed fix
resolveIp = '' ${parseIdentity} trap 'rm -f ${tfState}' EXIT ${decryptState} - host_ip=$(jq -r '.outputs.droplet_ipv4.value' ${tfState}) - rm -f ${tfState} + host_ip=$(jq -er '.outputs.droplet_ipv4.value' ${tfState}) || { + echo "Error: Could not resolve droplet_ipv4 from ${tfState}" >&2 + exit 1 + } '';,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/shell.nix` around lines 37 - 43, In resolveIp, fail fast if droplet_ipv4 is missing by changing the jq call that assigns host_ip from using the tolerant flag to the strict one (replace the current jq -r usage with jq -er so jq exits non‑zero on missing/null), and remove the redundant explicit rm -f ${tfState} at the end since the trap set earlier already cleans up; update references around parseIdentity, decryptState and tfState accordingly so host_ip is only set when jq succeeds.
28-30:⚠️ Potential issue | 🟠 MajorUnconditional decryption fails if age file is missing.
Unlike
decryptState,decryptVarsdoesn't check if${tfVars}.ageexists before attempting decryption. This will fail if the file is missing. Also, same restrictive permissions concern applies here.🔧 Proposed fix
decryptVars = '' - rage -d -i "$identity" ${tfVars}.age > ${tfVars} + if [ -f ${tfVars}.age ]; then + rm -f ${tfVars} + ( umask 077; rage -d -i "$identity" ${tfVars}.age > ${tfVars} ) + else + echo "Error: ${tfVars}.age not found" >&2 + exit 1 + fi '';,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/shell.nix` around lines 28 - 30, The decryptVars block unconditionally runs rage on ${tfVars}.age which fails if the .age file is missing and also doesn't enforce restrictive permissions; update the decryptVars logic (the decryptVars heredoc/variable) to first test for the existence of "${tfVars}.age" and only run rage when it exists, mirroring the pattern used by decryptState, and after writing the decrypted file set secure permissions (e.g., chmod 600) on ${tfVars} to avoid overly permissive files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 17-20: The CI workflow currently uses floating refs for
third-party actions; update the `uses` entries for
DeterminateSystems/nix-installer-action and
DeterminateSystems/magic-nix-cache-action to pinned versions (replace
`DeterminateSystems/nix-installer-action@main` with
`DeterminateSystems/nix-installer-action@v21` and
`DeterminateSystems/magic-nix-cache-action@main` with
`DeterminateSystems/magic-nix-cache-action@v13`), or pin them further to
specific commit SHAs for stronger reproducibility; leave `actions/checkout@v4`
as-is unless you also want to pin that to a SHA.
In @.github/workflows/integration-test.yml:
- Around line 101-112: The SSH retry loop's ssh invocation may hang on
interactive password prompts; update the ssh command inside the retry loop (the
until ... ssh ... true 2>/dev/null line) to include the option -o BatchMode=yes
so the client fails immediately on authentication prompts, making the retries
deterministic and preventing hangs; keep the existing -o
StrictHostKeyChecking=no and -o ConnectTimeout=5 options and add BatchMode=yes
alongside the identity file flag (-i ~/.ssh/id_ed25519).
- Around line 59-64: Replace writing the DO_TOKEN plaintext into
infra/terraform.tfvars and instead export it as an environment variable
TF_VAR_do_token (so Terraform picks it up without writing secrets to disk) and
remove the do_token assignment from the here-doc; additionally add a
cleanup-on-failure step that deletes infra/terraform.tfvars if it exists (or
ensure the job's post/finally step removes the workflow directory) to guarantee
no secret file remains on error. Use the variable names do_token,
TF_VAR_do_token and the file infra/terraform.tfvars to locate the changes.
In `@lib/shell.nix`:
- Around line 7-13: The parseIdentity snippet sets identity and unconditionally
runs "shift 2" when $1 is "-i" without checking $2, allowing argument injection
or empty identity; modify parseIdentity to verify that a second argument exists
before assigning identity and shifting (e.g., test [ -n "${2:-}" ]), and if it's
missing, emit an error/usage message and exit or fall back to the default
identity; update the block around the identity assignment and the shift 2 call
to perform that presence check and handle the missing-argument case.
- Around line 32-35: The encryptVars block (encryptVars) should first verify the
source file (${tfVars}) exists and exit/error if missing, then evaluate
${keysFile} roles.infra to collect recipients and validate that the recipient
list is non-empty (similar to encryptState) before invoking rage; if the
recipient list is empty, log/raise a clear error and do not run rage. Update the
encryptVars sequence to perform a file existence check for tfVars, capture the
output of nix eval roles.infra into a variable, assert that variable is
non-empty, and only then call rage to produce ${tfVars}.age, mirroring the
recipient validation used in the encryptState logic.
- Around line 21-26: The encryptState block currently pipes nix eval output into
rage without validating recipients, so if nix eval (resolving ${keysFile}
roles.infra) fails or returns an empty list rage will produce an unreadable
file; update encryptState to capture the output of nix eval into a variable,
check its exit status and that the resulting recipient string is non-empty
before invoking rage (referencing encryptState, ${tfState}, ${keysFile}, and
roles.infra), and if the check fails emit an error and abort the script rather
than running rage with zero recipients.
In `@modules/acme.nix`:
- Around line 16-21: The module currently sets security.acme when cfg.enable is
true but does not guard or document nginx's enableACME interaction; add either
an assertion in the acme module (e.g., in config / top-level check using
cfg.enable and referencing nginx virtualHost enableACME) that fails when an
nginx virtualHost has enableACME = true while omnix.acme (cfg.enable) is false,
or add a clear documentation comment in this module explaining that nginx
virtualHost enableACME requires omnix.acme.enable (cfg.enable) and that
security.acme.acceptTerms and defaults.email must be provided via this module;
reference the existing config block (lib.mkIf cfg.enable { security.acme = {
acceptTerms = true; defaults.email = cfg.email; }; }) and the nginx template
setting enableACME to implement the assertion or docs.
- Around line 10-13: The email option is currently required even when
omnix.acme.enable is false; change the lib.mkOption for email to accept null or
string and provide a default null (e.g., use lib.types.nullOr lib.types.str with
default = null) or add an assertion that checks that email is non-null only when
enable is true (use the enable flag in the assertion). Update the option
definition named email and any related assertion so evaluation succeeds when
enable = false and enforces a real string when enable = true.
In `@modules/services.nix`:
- Around line 136-148: The owner/group fallback logic is duplicated; extract a
shared let binding (e.g., owner and group) above the tmpfiles map and
services.logrotate.settings so both the tmpfiles mapping (map (dir: "d ${dir}
0770 ${owner} ${group} -") over dataDirs ++ logDirs) and the logrotate su
directive ("${if cfg.user != null then cfg.user else "root"} ${if cfg.group !=
null then cfg.group else "root"}") reference the same owner and group variables;
update the tmpfiles code and services.logrotate.settings to use these shared
owner and group bindings (keep the exact names owner and group to minimize
churn) so the fallback logic is defined once and reused.
- Around line 166-171: Add a short explanatory comment above the users.groups
block clarifying the intent of the conditional and branches: that the outer
condition users.groups = lib.mkIf (cfg.user != null && !cfg.dynamicUser ||
cfg.group != null) creates group entries when either an explicit cfg.group is
provided or when a non-dynamic cfg.user should have a matching group, and that
lib.optionalAttrs (cfg.group != null) { ${cfg.group} = { }; } handles the
explicit group case while the ${cfg.user} = { }; branch creates an implicit
user-named group; place the comment immediately above the users.groups
assignment and reference cfg.user, cfg.group, and cfg.dynamicUser so future
maintainers understand the branching.
In `@ROADMAP.md`:
- Around line 111-114: Clarify the mkIntegrationTest entry by specifying its
expected signature and invocation pattern: describe that
mkIntegrationTest(config: {services: ServiceConfig[], keys: KeyConfig, nodeName:
string, ...}) => {run: (flow: LifecycleFlow) => Promise<TestResult>, setup: ()
=> Promise<void>, teardown: () => Promise<void>} and add a one-line usage
example showing how a consumer calls mkIntegrationTest(...) to obtain the
harness and then calls harness.run(myLifecycleFlow). Reference
mkIntegrationTest, LifecycleFlow, TestResult, setup, teardown, and run so
readers know what the function returns and how to invoke it.
In `@templates/do-service/.github/workflows/ci.yml`:
- Around line 50-54: The nix eval for HOST_KEY can fail and abort the workflow;
change the HOST_KEY assignment and append logic so it is defensive: run
HOST_KEY=$(nix eval --raw --file keys.nix keys.host 2>/dev/null || echo "") and
then check that HOST_KEY is non-empty before echoing to ~/.ssh/known_hosts
(similar to the HOST_IP guard); optionally log or fail with a clear message if
HOST_IP is set but HOST_KEY is empty. Ensure references: HOST_IP, HOST_KEY,
keys.nix, and ~/.ssh/known_hosts are used in the updated checks.
---
Outside diff comments:
In `@modules/base.nix`:
- Around line 26-29: The module removed the default for the option stateVersion,
which breaks consumers that relied on "24.11"; restore a safe failure path by
adding an assertion in modules/base.nix guarding stateVersion: if the option is
unset/null/empty, assert with a clear error message that tells users to set
omnix.base.stateVersion (or update to the new templates/do-service/os.nix) and
optionally mention the previous default value ("24.11") as a hint; locate the
stateVersion lib.mkOption declaration and add the assertion immediately after it
so evaluation fails with the helpful message instead of an obscure error.
---
Duplicate comments:
In `@flake.nix`:
- Around line 97-104: The dev shell currently hardcodes packages in
devShells.default using pkgs.mkShell while the shellHook comes from
self.checks.${system}.git-hooks, which can also emit enabledPackages; replace or
augment the hardcoded package list (pkgs.nixfmt, pkgs.deadnix, pkgs.taplo) with
the enabledPackages exported by self.checks.${system}.git-hooks (e.g., use
enabledPackages from that check and merge it with any unavoidable extras) so
that additions in lib/hooks.nix like extraHooks or rustToolchain automatically
appear in the dev shell; update references in devShells.default to use
self.checks.${system}.git-hooks.enabledPackages together with shellHook.
In `@lib/bootstrap.nix`:
- Around line 18-24: The runtimeInputs list is missing pkgs.openssh while the
script calls ssh (via writeShellApplication), so add pkgs.openssh to the
runtimeInputs array alongside pkgs.rage, pkgs.jq, and pkgs.gnused; ensure it is
included unconditionally (or follow the same conditional pattern as ragenixPkg
if intended) so ssh is available at runtime and the writeShellApplication
sandbox lists it as a runtime dependency.
In `@lib/hooks.nix`:
- Around line 1-5: The function parameter list in lib/hooks.nix currently
swallows caller-provided pkgs via the catch-all "..." which discards { pkgs }
from callers; change the parameter list to explicitly accept pkgs (e.g., replace
"..., " with "pkgs" or "pkgs ? null" depending on whether it should be required)
and update any uses in the function to reference that pkgs symbol (keep
extraHooks and rustToolchain as before); this ensures callers passing { inherit
pkgs; } are not ignored and preserves explicit argument semantics.
In `@lib/shell.nix`:
- Around line 37-43: In resolveIp, fail fast if droplet_ipv4 is missing by
changing the jq call that assigns host_ip from using the tolerant flag to the
strict one (replace the current jq -r usage with jq -er so jq exits non‑zero on
missing/null), and remove the redundant explicit rm -f ${tfState} at the end
since the trap set earlier already cleans up; update references around
parseIdentity, decryptState and tfState accordingly so host_ip is only set when
jq succeeds.
- Around line 28-30: The decryptVars block unconditionally runs rage on
${tfVars}.age which fails if the .age file is missing and also doesn't enforce
restrictive permissions; update the decryptVars logic (the decryptVars
heredoc/variable) to first test for the existence of "${tfVars}.age" and only
run rage when it exists, mirroring the pattern used by decryptState, and after
writing the decrypted file set secure permissions (e.g., chmod 600) on ${tfVars}
to avoid overly permissive files.
In `@lib/terraform.nix`:
- Around line 22-27: The buildInputs list in lib/terraform.nix is missing
pkgs.nix which causes the runtime helpers encryptState and encryptVars (from
lib/shell.nix) to fail when they run nix eval; update the buildInputs expression
used by mkTask (runtimeInputs = buildInputs) to include pkgs.nix alongside
pkgs.terraform, pkgs.rage, and pkgs.jq, while preserving the conditional
inclusion of ragenixPkg so tfApply/tfDestroy/tfImport/tfRekey/rekey can invoke
the encryption helpers successfully.
In `@README.md`:
- Line 42: Add a short "Migration" note to the README explaining that the old
API function lib.mkRemote has been renamed to lib.mkGitHooks; state the exact
rename (lib.mkRemote → lib.mkGitHooks), list any signature changes (parameter
names, defaults, or removed/added options) and show the minimal migration steps
users must take (rename calls and update renamed options). Place this note near
the existing API table or in a new "Changelog / Migration" section so users
upgrading can quickly find it and cross-reference the symbols mkRemote and
mkGitHooks.
In `@templates/do-service/flake.nix`:
- Around line 35-43: The two deployConfig blocks duplicate package selection
inconsistently: one uses self.packages.x86_64-linux.default and the other uses
pkgs.hello; unify by extracting a single package binding (e.g., define a
variable like deployPackage or package = self.packages.x86_64-linux.default) and
reference that variable in both omnix.lib.mkDeploy calls (the deployConfig in
the flake-level and the per-system deployConfig), ensuring both the package
field passed into mkDeploy and any per-system package references use the same
identifier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 41e79b10-0845-43be-a20c-0429c734948a
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
.github/workflows/ci.yml.github/workflows/integration-test.ymlREADME.mdROADMAP.mdflake.nixlib/bootstrap.nixlib/default.nixlib/deploy.nixlib/hooks.nixlib/remote.nixlib/shell.nixlib/terraform.nixmodules/acme.nixmodules/base.nixmodules/digitalocean.nixmodules/disko.nixmodules/firewall.nixmodules/services.nixmodules/storage.nixtemplates/do-service/.github/workflows/ci.ymltemplates/do-service/config/secrets.nixtemplates/do-service/flake.nixtemplates/do-service/infra/secrets.nixtemplates/do-service/infra/terraform.tfvars.exampletemplates/do-service/infra/variables.tftemplates/do-service/keys.nixtemplates/do-service/os.nixtemplates/do-service/services.nix
💤 Files with no reviewable changes (1)
- lib/remote.nix
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Flake check
🧰 Additional context used
📓 Path-based instructions (2)
*
⚙️ CodeRabbit configuration file
Focus on providing constructive criticism. Whenever you see a suboptimal approach, suggest more idiomatic or robust alternative(s). Flag potential footguns. Suggest FP alternatives to mutable/imperative code. Point out architectural flaws like leaky abstractions, tight coupling, wrong level of abstraction, poor type modeling, over-abstraction, unclear domain boundaries. Code should generally be organized based on business concerns rather than technical aspects - suggest improvements if you find violations. Point out gaps in test coverage but suggest tests that are not too coupled to the implementation and actually test domain invariants and business logic.
Files:
README.mdROADMAP.mdflake.nix
**/*.md
⚙️ CodeRabbit configuration file
Focus on the contents of the docs and not on cosmetic things like markdown formatting. We use markdown files for various docs including but not limited to the north star system specification, SPEC.md, the plan for how to get there, ROADMAP.md, guidelines for AI contributors, AGENTS.md, project overview and instructions for human contributors, README.md. Think about the target audience of a document when deciding what comment to leave. For specifications and designs, suggest potential product, architectural, and UI/UX improvements. For plans, suggest changes that would make things more parallelizable and deliverable-focused. For instructions, suggest better rules and guidelines and point out missing instructions. In all cases, flag needless bloat, prefer clear concise writing, and consider the structure of the document and order of the sections
Files:
README.mdROADMAP.md
🔇 Additional comments (42)
modules/storage.nix (2)
10-13: ThevolumeNamevalidation concern was previously raised.The past review already flagged that
volumeNameaccepts arbitrary strings which could break device paths. That feedback still applies.
3-6: LGTM!Formatting refactor to multi-line
let ... inblock improves readability and aligns with the PR's consistent style.modules/base.nix (2)
62-62: The activation script vs. tmpfiles concern was previously raised.The past review already suggested using
systemd.tmpfiles.rulesinstead ofsystem.activationScriptsfor directory creation. That feedback still applies.
64-73: LGTM!Multi-line formatting with explicit
with pkgs;scope and++ cfg.extraPackagesconcatenation is clean and readable.templates/do-service/infra/terraform.tfvars.example (1)
1-2: LGTM!Clear instructions guiding users toward the age-encrypted workflow instead of plaintext secrets.
templates/do-service/infra/variables.tf (2)
6-9: LGTM!Good defensive validation to catch empty API tokens early in the Terraform plan phase rather than at API call time.
35-38: LGTM!Appropriate minimum constraint aligning with DigitalOcean's block storage requirements.
modules/digitalocean.nix (2)
12-15: LGTM!Helpful inline documentation clarifying NixOS module import semantics—imports are always evaluated but runtime config is properly gated by
lib.mkIf.
37-40: LGTM!Multi-line list formatting improves readability.
Also applies to: 53-58
lib/deploy.nix (2)
3-10: LGTM!Good addition of
targetSystemparameter with sensible default. This enables cross-architecture deployment scenarios while maintaining backward compatibility.Also applies to: 13-14
93-94: LGTM!Correct logic:
--remote-buildis appropriately added whenlocalSystemdiffers from the target (non-x86_64-linux), ensuring builds happen on the remote when cross-compiling isn't available locally.templates/do-service/services.nix (1)
4-4: LGTM!Service-specific directory name (
my-service) is more descriptive than the genericprodand follows a clearer naming convention.templates/do-service/infra/secrets.nix (1)
1-4: LGTM!Formatting refactor to multi-line
let ... inblock aligns with the PR's consistent style.lib/default.nix (2)
7-7: VerifymkGitHookssignature matches callers.The rename from
mkRemotetomkGitHooksaligns with the new git-hooks functionality. However, as noted in a previous review, callers may be passing unused parameters (e.g.,pkgs). Ensure the./hooks.nixsignature matches what callers provide.#!/bin/bash # Check how mkGitHooks is called and what parameters hooks.nix expects echo "=== Callers of mkGitHooks ===" rg -n "mkGitHooks" --type=nix -A 3 echo "" echo "=== hooks.nix parameter signature ===" head -20 lib/hooks.nix
1-1: LGTM!Using
...for additional attributes makes the interface more flexible for future extensions.templates/do-service/config/secrets.nix (1)
1-6: LGTM!The reformatting to multi-line
let...inis consistent with the broader Nix formatting patterns in this PR.modules/disko.nix (1)
17-22: LGTM!Good defensive assertion—fails early with a clear, actionable error message when the upstream
diskomodule is missing. This prevents confusing downstream errors and guides users to the correct solution.templates/do-service/.github/workflows/ci.yml (1)
34-34: LGTM!Good addition to support both
masterandmainas default branch names.README.md (1)
33-33: LGTM!Clear documentation that
omnix.nixosModules.defaultbundles upstream dependencies, reducing boilerplate for users.templates/do-service/keys.nix (1)
1-22: LGTM!The multi-line formatting for role arrays improves readability. The placeholder keys are clearly marked for replacement.
templates/do-service/os.nix (1)
1-11: LGTM!Good cleanup: removing unused
pkgsparameter and explicitly settingstateVersion = "24.11"aligns with the base module changes that removed the default. The nginx formatting improvements enhance readability.modules/firewall.nix (1)
3-26: LGTM!Clean use of
lib.optionalfor conditional port inclusion. The newenableHTTP/enableHTTPSoptions provide a semantic abstraction over raw port numbers with helpful descriptions (e.g., noting HTTP is needed for ACME challenges).lib/bootstrap.nix (1)
61-69: LGTM on the conditional rekeying refactor.The conditional block structure using
${ if ... then '' ... '' else "" }is clear and properly handles the optional ragenix rekeying step.lib/hooks.nix (1)
7-31: LGTM!The hook configuration is well-structured:
deadnixcorrectly excludes templates (which may contain intentionally unused bindings for demonstration), and the conditionalrustfmthook cleanly integrates when a Rust toolchain is provided. UsingdefaultHooks // extraHooksallows callers to override or extend hooks.lib/terraform.nix (1)
132-143: LGTM on theremotewrapper.The
remotescript correctly declarespkgs.opensshin its ownruntimeInputs, separate from the Terraform task inputs. This is the right approach since not all tasks need SSH.templates/do-service/flake.nix (1)
27-30: LGTM on module consolidation.Using
omnix.nixosModules.defaultinstead of importing individual modules simplifies the template. Per the context snippet (flake.nix:46-59), the default module transitively includes all required modules including upstream disko and ragenix.flake.nix (2)
44-57: LGTM on acme module integration.The new
acmemodule is properly gated behindlib.mkIf cfg.enable(per context snippet modules/acme.nix:16), so including it innixosModules.defaultwon't affect consumers who don't opt in.
86-88: Good defensive error handling for disko package.Using
or (throw "...")instead ofor nullprovides a clear failure message when disko isn't available for a given system, rather than silently producing null which could cause confusing downstream errors.modules/services.nix (2)
45-53: Good refactor tolib.optionalAttrs.The conditional serviceConfig attributes are now idiomatically expressed. This is cleaner than the previous
// (if cond then { ... } else { })pattern.
110-114: LGTM on the newlogDiroption.The
logDiroption with automatic tmpfiles directory creation and logrotate configuration is a useful addition that reduces boilerplate for service definitions.lib/shell.nix (1)
15-19: Force restrictive permissions on decrypted state file.The decrypted state file is written with caller-controlled permissions. Since Terraform state can contain secrets, enforce restrictive permissions.
ROADMAP.md (4)
3-21: LGTM!Clear documentation of resolved review findings. The checklist format is appropriate for tracking completed work.
23-38: LGTM!Good documentation of the refactoring status. The completed item aligns with the
lib/shell.nixchanges in this PR.
40-81: LGTM!The section addresses all previous review findings: generic workflow description instead of consumer-specific references, parallel paths in the diagram, explicit "Optional:" labeling for secretspec integration, and fixed "CLI interface" redundancy.
115-134: LGTM!Good separation of future nice-to-haves from completed milestones. The completed section provides useful context about the extraction history.
.github/workflows/integration-test.yml (7)
15-21: LGTM!Good concurrency configuration. Setting
cancel-in-progress: falseensures the cleanup step runs even if a new workflow is queued, preventing orphaned infrastructure.
42-48: LGTM!Correct SSH key setup with proper permissions and cleanup at the end of the workflow.
94-99: LGTM!Good practice masking the IP address in workflow logs via
::add-mask::.
136-147: LGTM!Good approach: fetch the real host key after bootstrap, update
keys.nixfor future deploys, and refreshknown_hostsfor strict host key verification in subsequent SSH connections.
149-171: LGTM!Good verification sequence: SSH connectivity, OS identity, and firewall state. The fallback warning for nftables acknowledges the limitation gracefully.
173-180: LGTM!Proper cleanup with
if: always()ensures infrastructure teardown even on failure, preventing orphaned resources and cloud cost leakage.
114-120: No action needed. The flake configuration name correctly remainsmy-service.The sed commands on lines 83-84 modify the content within
infra/main.tfandos.nix(e.g., hostnames, resource identifiers), but they do not modifyflake.nix. SinceprojectName = "my-service"is hardcoded in the template's flake.nix (line 21) and not affected by the sed substitutions, thenixosConfigurations.my-serviceattribute is correctly referenced as.#my-serviceon line 117.> Likely an incorrect or invalid review comment.
| cat > infra/terraform.tfvars <<'VARS' | ||
| do_token = "${{ secrets.DO_TOKEN }}" | ||
| ssh_key_name = "omnix-integration-test" | ||
| region = "${{ inputs.region }}" | ||
| droplet_size = "${{ inputs.droplet_size }}" | ||
| VARS |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Plaintext secrets in terraform.tfvars.
The DO_TOKEN is written in plaintext to terraform.tfvars. While acceptable for CI (file is ephemeral), ensure the workflow directory is cleaned up on failure. Consider using environment variables with TF_VAR_do_token instead to avoid writing secrets to disk.
🛡️ Alternative using environment variables
+ - name: Set Terraform vars
+ run: |
+ echo "TF_VAR_do_token=${{ secrets.DO_TOKEN }}" >> "$GITHUB_ENV"
- name: Scaffold test project from template
run: |
# ... existing scaffold steps ...
- # Write test terraform vars (encrypted in real usage, plaintext here)
- cat > infra/terraform.tfvars <<'VARS'
- do_token = "${{ secrets.DO_TOKEN }}"
- ssh_key_name = "omnix-integration-test"
- region = "${{ inputs.region }}"
- droplet_size = "${{ inputs.droplet_size }}"
- VARS
+ # Write non-sensitive terraform vars (DO_TOKEN via TF_VAR_do_token env)
+ cat > infra/terraform.tfvars <<VARS
+ ssh_key_name = "omnix-integration-test"
+ region = "${{ inputs.region }}"
+ droplet_size = "${{ inputs.droplet_size }}"
+ VARS🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/integration-test.yml around lines 59 - 64, Replace writing
the DO_TOKEN plaintext into infra/terraform.tfvars and instead export it as an
environment variable TF_VAR_do_token (so Terraform picks it up without writing
secrets to disk) and remove the do_token assignment from the here-doc;
additionally add a cleanup-on-failure step that deletes infra/terraform.tfvars
if it exists (or ensure the job's post/finally step removes the workflow
directory) to guarantee no secret file remains on error. Use the variable names
do_token, TF_VAR_do_token and the file infra/terraform.tfvars to locate the
changes.
| - name: Wait for SSH | ||
| run: | | ||
| retries=0 | ||
| until ssh -o StrictHostKeyChecking=no -o ConnectTimeout=5 \ | ||
| -i ~/.ssh/id_ed25519 "root@$HOST_IP" true 2>/dev/null; do | ||
| retries=$((retries + 1)) | ||
| if [ "$retries" -ge 30 ]; then | ||
| echo "SSH not available after 2.5 minutes" >&2 | ||
| exit 1 | ||
| fi | ||
| sleep 5 | ||
| done |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding BatchMode=yes to SSH options.
Adding BatchMode=yes prevents the SSH client from hanging on unexpected password prompts or interactive authentication, making the retry loop more robust.
🔧 Proposed fix
- name: Wait for SSH
run: |
retries=0
- until ssh -o StrictHostKeyChecking=no -o ConnectTimeout=5 \
+ until ssh -o StrictHostKeyChecking=no -o ConnectTimeout=5 -o BatchMode=yes \
-i ~/.ssh/id_ed25519 "root@$HOST_IP" true 2>/dev/null; do📝 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.
| - name: Wait for SSH | |
| run: | | |
| retries=0 | |
| until ssh -o StrictHostKeyChecking=no -o ConnectTimeout=5 \ | |
| -i ~/.ssh/id_ed25519 "root@$HOST_IP" true 2>/dev/null; do | |
| retries=$((retries + 1)) | |
| if [ "$retries" -ge 30 ]; then | |
| echo "SSH not available after 2.5 minutes" >&2 | |
| exit 1 | |
| fi | |
| sleep 5 | |
| done | |
| - name: Wait for SSH | |
| run: | | |
| retries=0 | |
| until ssh -o StrictHostKeyChecking=no -o ConnectTimeout=5 -o BatchMode=yes \ | |
| -i ~/.ssh/id_ed25519 "root@$HOST_IP" true 2>/dev/null; do | |
| retries=$((retries + 1)) | |
| if [ "$retries" -ge 30 ]; then | |
| echo "SSH not available after 2.5 minutes" >&2 | |
| exit 1 | |
| fi | |
| sleep 5 | |
| done |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/integration-test.yml around lines 101 - 112, The SSH retry
loop's ssh invocation may hang on interactive password prompts; update the ssh
command inside the retry loop (the until ... ssh ... true 2>/dev/null line) to
include the option -o BatchMode=yes so the client fails immediately on
authentication prompts, making the retries deterministic and preventing hangs;
keep the existing -o StrictHostKeyChecking=no and -o ConnectTimeout=5 options
and add BatchMode=yes alongside the identity file flag (-i ~/.ssh/id_ed25519).
| parseIdentity = '' | ||
| identity=~/.ssh/id_ed25519 | ||
| if [ "''${1:-}" = "-i" ]; then | ||
| identity="$2" | ||
| shift 2 | ||
| fi | ||
| ''; |
There was a problem hiding this comment.
parseIdentity allows arbitrary argument injection.
The shift 2 unconditionally runs after checking only $1, but doesn't validate that $2 actually exists. If called with -i but no second argument, $identity becomes empty and subsequent commands may fail confusingly or use unintended defaults.
🛡️ Proposed fix
parseIdentity = ''
identity=~/.ssh/id_ed25519
if [ "''${1:-}" = "-i" ]; then
+ if [ -z "''${2:-}" ]; then
+ echo "Error: -i requires an identity file argument" >&2
+ exit 1
+ fi
identity="$2"
shift 2
fi
'';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/shell.nix` around lines 7 - 13, The parseIdentity snippet sets identity
and unconditionally runs "shift 2" when $1 is "-i" without checking $2, allowing
argument injection or empty identity; modify parseIdentity to verify that a
second argument exists before assigning identity and shifting (e.g., test [ -n
"${2:-}" ]), and if it's missing, emit an error/usage message and exit or fall
back to the default identity; update the block around the identity assignment
and the shift 2 call to perform that presence check and handle the
missing-argument case.
| encryptState = '' | ||
| if [ -f ${tfState} ]; then | ||
| nix eval --raw --file ${keysFile} roles.infra --apply 'builtins.concatStringsSep "\n"' \ | ||
| | rage -e -R /dev/stdin -o ${tfState}.age ${tfState} | ||
| fi | ||
| ''; |
There was a problem hiding this comment.
Silent failure if key resolution fails.
If nix eval fails (e.g., missing roles.infra in keysFile), the pipeline silently continues with no recipients. rage will encrypt to zero recipients, producing a file that cannot be decrypted. Consider validating the key list before encryption.
🛡️ Proposed fix
encryptState = ''
if [ -f ${tfState} ]; then
- nix eval --raw --file ${keysFile} roles.infra --apply 'builtins.concatStringsSep "\n"' \
- | rage -e -R /dev/stdin -o ${tfState}.age ${tfState}
+ recipients=$(nix eval --raw --file ${keysFile} roles.infra --apply 'builtins.concatStringsSep "\n"')
+ if [ -z "$recipients" ]; then
+ echo "Error: No recipients found in ${keysFile} roles.infra" >&2
+ exit 1
+ fi
+ echo "$recipients" | rage -e -R /dev/stdin -o ${tfState}.age ${tfState}
fi
'';📝 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.
| encryptState = '' | |
| if [ -f ${tfState} ]; then | |
| nix eval --raw --file ${keysFile} roles.infra --apply 'builtins.concatStringsSep "\n"' \ | |
| | rage -e -R /dev/stdin -o ${tfState}.age ${tfState} | |
| fi | |
| ''; | |
| encryptState = '' | |
| if [ -f ${tfState} ]; then | |
| recipients=$(nix eval --raw --file ${keysFile} roles.infra --apply 'builtins.concatStringsSep "\n"') | |
| if [ -z "$recipients" ]; then | |
| echo "Error: No recipients found in ${keysFile} roles.infra" >&2 | |
| exit 1 | |
| fi | |
| echo "$recipients" | rage -e -R /dev/stdin -o ${tfState}.age ${tfState} | |
| fi | |
| ''; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/shell.nix` around lines 21 - 26, The encryptState block currently pipes
nix eval output into rage without validating recipients, so if nix eval
(resolving ${keysFile} roles.infra) fails or returns an empty list rage will
produce an unreadable file; update encryptState to capture the output of nix
eval into a variable, check its exit status and that the resulting recipient
string is non-empty before invoking rage (referencing encryptState, ${tfState},
${keysFile}, and roles.infra), and if the check fails emit an error and abort
the script rather than running rage with zero recipients.
| config = lib.mkIf cfg.enable { | ||
| security.acme = { | ||
| acceptTerms = true; | ||
| defaults.email = cfg.email; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider documenting the relationship with nginx ACME.
The template at templates/do-service/os.nix configures nginx with enableACME = true independently of this module. If a user sets enableACME on an nginx virtualHost without enabling omnix.acme, they'll get NixOS's default ACME behavior (which requires security.acme.acceptTerms and email to be set elsewhere). Consider adding an assertion or documentation clarifying that omnix.acme.enable should be set when using ACME features.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/acme.nix` around lines 16 - 21, The module currently sets
security.acme when cfg.enable is true but does not guard or document nginx's
enableACME interaction; add either an assertion in the acme module (e.g., in
config / top-level check using cfg.enable and referencing nginx virtualHost
enableACME) that fails when an nginx virtualHost has enableACME = true while
omnix.acme (cfg.enable) is false, or add a clear documentation comment in this
module explaining that nginx virtualHost enableACME requires omnix.acme.enable
(cfg.enable) and that security.acme.acceptTerms and defaults.email must be
provided via this module; reference the existing config block (lib.mkIf
cfg.enable { security.acme = { acceptTerms = true; defaults.email = cfg.email;
}; }) and the nginx template setting enableACME to implement the assertion or
docs.
| - [ ] `mkIntegrationTest` lib function -- let consumers define their own | ||
| lifecycle test flows using the same harness, parameterized by their | ||
| project-specific config (services, keys, node name) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify the mkIntegrationTest deliverable.
The description mentions parameterization by "project-specific config (services, keys, node name)" but doesn't specify what the function returns or how consumers would invoke it. Consider adding expected signature or usage pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ROADMAP.md` around lines 111 - 114, Clarify the mkIntegrationTest entry by
specifying its expected signature and invocation pattern: describe that
mkIntegrationTest(config: {services: ServiceConfig[], keys: KeyConfig, nodeName:
string, ...}) => {run: (flow: LifecycleFlow) => Promise<TestResult>, setup: ()
=> Promise<void>, teardown: () => Promise<void>} and add a one-line usage
example showing how a consumer calls mkIntegrationTest(...) to obtain the
harness and then calls harness.run(myLifecycleFlow). Reference
mkIntegrationTest, LifecycleFlow, TestResult, setup, teardown, and run so
readers know what the function returns and how to invoke it.
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
modules/firewall.nix (1)
13-27: 🧹 Nitpick | 🔵 TrivialAdd module tests for the new port-toggle invariants.
Please add coverage for combinations of
enable,enableHTTP,enableHTTPS, and customallowedTCPPortsto lock in the expected invariant: SSH always present when enabled, and 80/443 only when toggled.As per coding guidelines,
*: "Point out gaps in test coverage ... test domain invariants and business logic."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/firewall.nix` around lines 13 - 27, Add unit/module tests that assert the firewall.allowedTCPPorts list produced by this module respects the invariants: when cfg.enable is true the list always contains 22 (SSH), port 80 is present iff cfg.enableHTTP is true, port 443 is present iff cfg.enableHTTPS is true, and any entries from cfg.allowedTCPPorts are appended; conversely when cfg.enable is false the module should not enable networking.firewall. Create test cases that exercise combinations of enable=true/false with enableHTTP=true/false, enableHTTPS=true/false and non-empty allowedTCPPorts to validate the exact resulting list produced by the config expression that sets networking.firewall.allowedTCPPorts (the expression using [ 22 ] ++ httpPorts ++ httpsPorts ++ cfg.allowedTCPPorts).templates/do-service/flake.nix (1)
53-58: 🧹 Nitpick | 🔵 Trivial
systemparameter is unused bymkTerraform.Per the function signature in
lib/terraform.nix,systemis absorbed by...and not used internally—the function only needspkgs,keysFile, and optionalragenixPkg/secretsRules. Passing it is harmless but misleading.♻️ Remove unused parameter
infraPkgs = omnix.lib.mkTerraform { - inherit pkgs system; + inherit pkgs; keysFile = ./keys.nix; ragenixPkg = omnix.inputs.ragenix.packages.${system}.default; secretsRules = ./config/secrets.nix; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/do-service/flake.nix` around lines 53 - 58, The call to omnix.lib.mkTerraform is mistakenly passing the unused parameter system via "inherit pkgs system"; update the mkTerraform invocation (infraPkgs) to only pass the required arguments (pkgs, keysFile, and the optional ragenixPkg and secretsRules) by removing system from the inherit list (i.e., use "inherit pkgs;") so mkTerraform is not given the unused system parameter while keeping ragenixPkg = omnix.inputs.ragenix.packages.${system}.default and secretsRules unchanged.
♻️ Duplicate comments (9)
modules/storage.nix (1)
10-13:⚠️ Potential issue | 🟠 MajorConstrain
volumeNameto a safe character set before path interpolation.
volumeNameis still an unrestricted string but is used in a device path (Line 24). Invalid characters can break mounts or generate invalid paths.Proposed fix
volumeName = lib.mkOption { - type = lib.types.str; + type = lib.types.strMatching "^[A-Za-z0-9._-]+$"; description = "DigitalOcean volume name (used in /dev/disk/by-id/scsi-0DO_Volume_<name>)"; };Also applies to: 24-24
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/storage.nix` around lines 10 - 13, The option volumeName must be constrained to a safe character set before it’s used in the device path; update the volumeName mkOption to include a validate function that rejects any value not matching a safe regex (e.g. ^[A-Za-z0-9._-]+$) using builtins.match and throw a clear error message, and keep using the validated volumeName where the device path is interpolated so only allowed characters can reach that path construction.lib/shell.nix (3)
9-11:⚠️ Potential issue | 🟡 MinorValidate
-iargument beforeshift 2.Line 9 checks only
$1; if-iis passed without a value, identity handling becomes invalid and error behavior is opaque.Suggested fix
parseIdentity = '' identity=~/.ssh/id_ed25519 if [ "''${1:-}" = "-i" ]; then + if [ -z "''${2:-}" ]; then + echo "Error: -i requires an identity file argument" >&2 + exit 1 + fi identity="$2" shift 2 fi '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/shell.nix` around lines 9 - 11, The script checks for the -i flag then immediately consumes two args (identity="$2" and shift 2) which breaks if -i is passed without a value; update the branch that handles the -i flag to validate that the second positional parameter exists (check "${2:-}" or use -z) before assigning to the identity variable and calling shift 2, and if the value is missing print a clear error and exit non‑zero; keep references to the identity assignment (identity="$2") and the shift 2 call so you modify that exact block.
24-26:⚠️ Potential issue | 🟠 MajorGuard
encryptStateagainst empty/failed recipient resolution.If
nix evalfails or resolves no recipients, the current pipeline can still produce a broken encrypted file.Suggested fix
encryptState = '' if [ -f ${tfState} ]; then - nix eval --raw --file ${keysFile} roles.infra --apply 'builtins.concatStringsSep "\n"' \ - | rage -e -R /dev/stdin -o ${tfState}.age ${tfState} + recipients=$(nix eval --raw --file ${keysFile} roles.infra --apply 'builtins.concatStringsSep "\n"') || { + echo "Error: failed to resolve roles.infra from ${keysFile}" >&2 + exit 1 + } + if [ -z "$recipients" ]; then + echo "Error: no recipients found in ${keysFile} roles.infra" >&2 + exit 1 + fi + echo "$recipients" | rage -e -R /dev/stdin -o ${tfState}.age ${tfState} fi '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/shell.nix` around lines 24 - 26, The pipeline that resolves recipients via "nix eval --file ${keysFile} roles.infra ..." must be validated before attempting to encrypt: capture the output of that command into a variable (used by encryptState), check that it succeeded and is non-empty, and if it failed or produced no recipients, print an error and exit non-zero instead of piping to rage; update the logic around encryptState/tfState to only call rage when the recipients string is non-empty and to propagate nix eval errors (e.g., check its exit status or test -n "$recipients"), referencing the existing symbols: nix eval (roles.infra), ${keysFile}, ${tfState}, and the encrypt step that invokes rage.
34-37:⚠️ Potential issue | 🟠 MajorAdd source-file and recipient validation in
encryptVars.
encryptVarscurrently skips both${tfVars}existence checks and recipient validation, so failure modes are silent/unhelpful.Suggested fix
encryptVars = '' - nix eval --raw --file ${keysFile} roles.infra --apply 'builtins.concatStringsSep "\n"' \ - | rage -e -R /dev/stdin -o ${tfVars}.age ${tfVars} + if [ ! -f ${tfVars} ]; then + echo "Error: ${tfVars} not found" >&2 + exit 1 + fi + recipients=$(nix eval --raw --file ${keysFile} roles.infra --apply 'builtins.concatStringsSep "\n"') || { + echo "Error: failed to resolve roles.infra from ${keysFile}" >&2 + exit 1 + } + if [ -z "$recipients" ]; then + echo "Error: no recipients found in ${keysFile} roles.infra" >&2 + exit 1 + fi + echo "$recipients" | rage -e -R /dev/stdin -o ${tfVars}.age ${tfVars} '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/shell.nix` around lines 34 - 37, encryptVars currently pipes data into rage without validating that the source file (${tfVars}) exists or that recipients from ${keysFile} are valid; update the encryptVars block to first check that ${tfVars} exists and is non-empty and that the recipient list parsed from ${keysFile} (roles.infra) yields at least one valid recipient, failing with a clear error message via builtins.abort or nix's builtins.trace if checks fail, then proceed to run the nix eval | rage pipeline only when validations pass; reference the symbols encryptVars, keysFile, roles.infra and ${tfVars} when adding these guards so the logic is easy to locate.modules/acme.nix (1)
10-13:⚠️ Potential issue | 🟠 MajorMake
Line 10 currently makes
omnix.acme.emailmandatory even whenomnix.acme.enable = false, which can cause unnecessary evaluation failures for users not using ACME.Suggested fix
options.omnix.acme = { @@ email = lib.mkOption { - type = lib.types.str; + type = lib.types.nullOr lib.types.str; + default = null; description = "Email address for ACME renewal notifications"; }; }; config = lib.mkIf cfg.enable { + assertions = [ + { + assertion = cfg.email != null && cfg.email != ""; + message = "omnix.acme.email must be set when omnix.acme.enable is true"; + } + ]; security.acme = { acceptTerms = true; defaults.email = cfg.email; }; };Also applies to: 16-21
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/acme.nix` around lines 10 - 13, The email option is currently required even when ACME is disabled; change the omnix.acme.email option to be nullable and only required when omnix.acme.enable is true by setting its type to a nullable string (e.g., lib.types.nullOr lib.types.str) and providing a default null, then add a validation/assertion in the module config that enforces config.omnix.acme.email is non-null only if config.omnix.acme.enable is true (apply the same pattern to the other options referenced around lines 16-21)..github/workflows/ci.yml (1)
18-19:⚠️ Potential issue | 🟡 MinorPin third-party actions to immutable refs.
Using
@maincan break CI unpredictably and weakens supply-chain reproducibility.#!/bin/bash # Read-only verification: detect floating refs for third-party actions rg -nP 'DeterminateSystems/(nix-installer-action|magic-nix-cache-action)@main' .github/workflows templates/do-service/.github/workflows -g '*.yml'As per coding guidelines,
*: "Flag potential footguns" and suggest robust alternatives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 18 - 19, The workflow uses floating refs for third-party actions DeterminateSystems/nix-installer-action@main and DeterminateSystems/magic-nix-cache-action@main; update both usages to immutable refs (replace `@main` with a specific release tag or commit SHA for each action) to ensure CI reproducibility and supply-chain stability, and re-run the provided verification command (rg -nP 'DeterminateSystems/(nix-installer-action|magic-nix-cache-action)@main' ...) to confirm no occurrences remain.templates/do-service/.github/workflows/ci.yml (1)
52-53:⚠️ Potential issue | 🟡 MinorGuard
HOST_KEYevaluation to avoid hard-failing deploy bootstrap.
nix eval --raw --file keys.nix keys.hostis still unguarded; ifkeys.nixis missing/malformed, the deploy step exits unexpectedly.As per coding guidelines, `*`: "Flag potential footguns" and suggest robust alternatives.🛡️ Suggested defensive fix
- HOST_KEY=$(nix eval --raw --file keys.nix keys.host) - echo "$HOST_IP $HOST_KEY" >> ~/.ssh/known_hosts + HOST_KEY=$(nix eval --raw --file keys.nix keys.host 2>/dev/null || echo "") + if [ -n "$HOST_KEY" ]; then + echo "$HOST_IP $HOST_KEY" >> ~/.ssh/known_hosts + else + echo "Warning: HOST_IP resolved but keys.host is missing/unreadable" >&2 + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/do-service/.github/workflows/ci.yml` around lines 52 - 53, The HOST_KEY evaluation is unguarded and can hard-fail deployment if `nix eval --raw --file keys.nix keys.host` fails; change the step to capture the command into a variable (e.g., HOST_KEY=$(nix eval --raw --file keys.nix keys.host 2>/dev/null) or using set +e and capture exit code), verify the command succeeded and HOST_KEY is non-empty before echoing "$HOST_IP $HOST_KEY" >> ~/.ssh/known_hosts, and on failure emit a warning log and skip adding the host instead of letting the job exit (use conditional checks on the exit code or empty string and avoid unhandled failures).README.md (1)
42-42:⚠️ Potential issue | 🟠 MajorAdd a short migration note for
mkRemote→mkGitHooks.The table update is good, but existing users still need a concrete before/after snippet and any argument/default changes to migrate safely.
As per coding guidelines,
**/*.md: "point out missing instructions" and "consider the structure of the document and order of the sections."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 42, Add a short "Migration" subsection explaining how to replace lib.mkRemote usage with lib.mkGitHooks: show a minimal before snippet using mkRemote and the equivalent after snippet using mkGitHooks, list any changed or removed arguments and their new names or defaults (e.g., optionX -> optionY, default behavior changes), and include a note about optional rustfmt behavior if relevant; place this note near the table entry for lib.mkGitHooks so users see the concrete before/after example and argument/default changes when scanning the docs.lib/default.nix (1)
1-1: 🧹 Nitpick | 🔵 TrivialPrefer explicit argument sets at this public lib boundary.
Using
...here makes the interface less strict and can mask accidental input drift. Prefer explicit attrs unless passthrough is intentionally required and documented.As per coding guidelines, `*`: "Point out architectural flaws like leaky abstractions."♻️ Suggested interface tightening
-{ deploy-rs, nixos-anywhere, ... }: +{ deploy-rs, nixos-anywhere }:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/default.nix` at line 1, The public function currently accepts an open argument set "{ deploy-rs, nixos-anywhere, ... }", which weakens the API; replace the variadic "..." with an explicit list of accepted attributes (e.g. remove "..." and explicitly declare any optional attrs with defaults) or if passthrough is required, add a documented single explicit passthrough attr (e.g. passthrough ? {}) and remove the open ellipsis; update the function signature where "{ deploy-rs, nixos-anywhere, ... }" appears and document any intentionally allowed extra keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 8-10: The global workflow permissions block currently sets
id-token: write which is unnecessary for the nix flake check and expands
credential scope; change the global permissions to only include contents: read
and remove id-token, then add id-token: write (or the narrower OIDC permission
needed) to the specific job(s) that actually perform OIDC actions (e.g., the job
running nix flake check or any deploy/publish jobs) so only those jobs have the
elevated token scope; update the permissions keys referenced in the workflow
(permissions, id-token, contents) and any relevant job-level permissions blocks
to grant id-token where required.
In @.github/workflows/integration-test.yml:
- Around line 131-134: The teardown step currently silences failures by
appending "|| true" to the run command, which can hide cleanup errors; edit the
Teardown job's run command (the line invoking "nix run .#tfDestroy -- -i
~/.ssh/id_ed25519") to remove "|| true" so failures in tfDestroy propagate and
cause the job to fail (since the job already uses "if: always()"), ensuring
cleanup errors are visible and not ignored.
- Around line 5-13: The declared workflow_dispatch inputs inputs.droplet_size
and inputs.region are never used; either remove those inputs or wire them into
the Terraform steps by exporting them as TF vars before plan/apply. Update the
job steps that run the nix/terraform commands (the steps that perform terraform
plan/apply) to set environment variables TF_VAR_droplet_size and TF_VAR_region
from ${{ inputs.droplet_size }} and ${{ inputs.region }} (or pass them into the
nix command environment) so Terraform sees the values; alternatively, delete the
inputs.droplet_size and inputs.region blocks if you choose not to use them.
- Around line 34-40: Replace the floating branch refs for the DeterminateSystems
actions with immutable references: change the two usages of
DeterminateSystems/nix-installer-action@main and
DeterminateSystems/magic-nix-cache-action@main to pinned commit SHAs or stable
release tags (e.g., DeterminateSystems/nix-installer-action@<commit-sha> and
DeterminateSystems/magic-nix-cache-action@<commit-sha>) so the workflow uses
fixed versions and avoids supply-chain risk when running with id-token: write
and secrets.SSH_KEY.
In `@modules/disko.nix`:
- Around line 17-21: The assertion currently uses builtins.hasAttr "disko"
config inside the assertions block; replace that runtime-config check with an
options namespace test by using the idiomatic guard options ? disko so the
module detects the disko option declarations rather than merged config
values—update the assertion expression (the entry named assertion in the
assertions list) to use options ? disko and keep the existing message text.
In `@modules/services.nix`:
- Around line 48-53: The service unit currently only grants the dynamic service
user access via ReadWritePaths = [ svcCfg.dataDir ] while the module creates
logDir via tmpfiles as root:root, so add svcCfg.logDir to the serviceConfig
ReadWritePaths (i.e. include svcCfg.logDir alongside svcCfg.dataDir in
ReadWritePaths) or instead replace the tmpfiles-managed directory with a
systemd-managed directory by using LogsDirectory=svcCfg.logDir (or the
appropriate systemd helper) so the dynamicUser can access the log directory;
update the serviceConfig block that sets ReadWritePaths and/or switch to
LogsDirectory to ensure permissions for dynamicUser.
- Around line 14-25: Replace the string-joined ExecStart with a list form so
arguments are escaped correctly: instead of building a single string using
builtins.concatStringsSep and svcCfg.extraArgs, set serviceConfig.ExecStart (or
the local execStart variable) to a list like [ path "--config" configFile ] ++
svcCfg.extraArgs so NixOS will escape each element (via
lib.strings.escapeShellArgs) and preserve argument boundaries; apply the same
change to the other identical ExecStart occurrence referenced in the comment.
In `@ROADMAP.md`:
- Around line 3-22: The "PR review findings" list in ROADMAP.md is a growing,
completed-items section; either move these checked items into a new CHANGELOG.md
(create CHANGELOG.md and append the current list under a dated entry, then
replace the list in ROADMAP.md with a short link/note pointing to that
changelog) or collapse them inline using a Markdown collapsible block (wrap the
existing "PR review findings" list inside a <details>...</details> with a
concise summary heading) so ROADMAP.md only shows actionable, forward-looking
items; update ROADMAP.md to reference the new CHANGELOG or the collapsed block
accordingly.
- Around line 132-133: Update the checklist line to reflect current library
exports: replace the outdated mention of mkRemote with the new mkGitHooks and
adjust the phrasing to note that shell helpers were consolidated (lib/remote.nix
removed, helpers moved into shell helper export mkGitHooks). Concretely, change
the checked item that lists "mkTerraform, mkDeploy, mkBootstrap, mkRemote" to
"mkTerraform, mkDeploy, mkBootstrap, mkGitHooks" and, if helpful, add a brief
parenthetical that shell helpers were consolidated into the shell helper export
(mkGitHooks) to avoid future confusion.
In `@templates/do-service/.github/workflows/integration-test.yml`:
- Around line 89-91: The Teardown step ("Teardown") masks failures by appending
"|| true" to the run command; remove the trailing "|| true" from the "nix run
.#tfDestroy -- -i ~/.ssh/id_ed25519 || true" invocation so the step fails on
errors (keep the existing "if: always()" to ensure it still runs), allowing
workflow failure when destroy fails and preventing orphaned droplets.
- Around line 5-13: The workflow defines dispatch inputs droplet_size and region
but never passes them to Terraform; update the CI steps that run tfPlan/tfApply
to export those inputs as environment variables (TF_VAR_droplet_size and
TF_VAR_region) or create a generated tfvars file before calling tfPlan/tfApply
so Terraform receives the UI-supplied values; locate the inputs named
droplet_size and region and wire them into the step that invokes tfPlan/tfApply
(or the job that runs Terraform) by setting TF_VAR_droplet_size and
TF_VAR_region from the workflow inputs or writing a .tfvars file using those
inputs.
- Around line 31-37: Replace the floating refs for the DeterminateSystems
actions with immutable refs: change DeterminateSystems/nix-installer-action@main
and DeterminateSystems/magic-nix-cache-action@main to specific release tags or
commit SHAs (e.g., DeterminateSystems/nix-installer-action@vX.Y.Z or
@<commit-sha>) so the workflow's behavior cannot be altered by upstream
force-pushes; update both action references to use stable tags/SHAs consistent
with how actions/checkout@v4 is pinned.
In `@templates/do-service/infra/variables.tf`:
- Around line 35-38: The validation for variable volume_size_gb allows
fractional GBs; update the validation block for volume_size_gb so it enforces a
whole-number GB >= 1 by changing the condition to check both >= 1 and equality
with its floored value (e.g., use var.volume_size_gb >= 1 && var.volume_size_gb
== floor(var.volume_size_gb)), and update error_message to reflect
"volume_size_gb must be an integer >= 1."; target the validation block for the
variable named volume_size_gb.
- Around line 6-9: The current validation on the variable do_token only checks
length(var.do_token) > 0 and will accept whitespace-only values; update the
variable block for do_token to (1) set nullable = false on the variable and (2)
change the validation condition to use trimmed input e.g.
length(trimspace(var.do_token)) > 0 so whitespace-only strings fail, and update
the error_message accordingly; locate the variable declaration and its
validation block in variables.tf (variable "do_token" / validation { ... }) and
apply these changes.
---
Outside diff comments:
In `@modules/firewall.nix`:
- Around line 13-27: Add unit/module tests that assert the
firewall.allowedTCPPorts list produced by this module respects the invariants:
when cfg.enable is true the list always contains 22 (SSH), port 80 is present
iff cfg.enableHTTP is true, port 443 is present iff cfg.enableHTTPS is true, and
any entries from cfg.allowedTCPPorts are appended; conversely when cfg.enable is
false the module should not enable networking.firewall. Create test cases that
exercise combinations of enable=true/false with enableHTTP=true/false,
enableHTTPS=true/false and non-empty allowedTCPPorts to validate the exact
resulting list produced by the config expression that sets
networking.firewall.allowedTCPPorts (the expression using [ 22 ] ++ httpPorts ++
httpsPorts ++ cfg.allowedTCPPorts).
In `@templates/do-service/flake.nix`:
- Around line 53-58: The call to omnix.lib.mkTerraform is mistakenly passing the
unused parameter system via "inherit pkgs system"; update the mkTerraform
invocation (infraPkgs) to only pass the required arguments (pkgs, keysFile, and
the optional ragenixPkg and secretsRules) by removing system from the inherit
list (i.e., use "inherit pkgs;") so mkTerraform is not given the unused system
parameter while keeping ragenixPkg =
omnix.inputs.ragenix.packages.${system}.default and secretsRules unchanged.
---
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 18-19: The workflow uses floating refs for third-party actions
DeterminateSystems/nix-installer-action@main and
DeterminateSystems/magic-nix-cache-action@main; update both usages to immutable
refs (replace `@main` with a specific release tag or commit SHA for each action)
to ensure CI reproducibility and supply-chain stability, and re-run the provided
verification command (rg -nP
'DeterminateSystems/(nix-installer-action|magic-nix-cache-action)@main' ...) to
confirm no occurrences remain.
In `@lib/default.nix`:
- Line 1: The public function currently accepts an open argument set "{
deploy-rs, nixos-anywhere, ... }", which weakens the API; replace the variadic
"..." with an explicit list of accepted attributes (e.g. remove "..." and
explicitly declare any optional attrs with defaults) or if passthrough is
required, add a documented single explicit passthrough attr (e.g. passthrough ?
{}) and remove the open ellipsis; update the function signature where "{
deploy-rs, nixos-anywhere, ... }" appears and document any intentionally allowed
extra keys.
In `@lib/shell.nix`:
- Around line 9-11: The script checks for the -i flag then immediately consumes
two args (identity="$2" and shift 2) which breaks if -i is passed without a
value; update the branch that handles the -i flag to validate that the second
positional parameter exists (check "${2:-}" or use -z) before assigning to the
identity variable and calling shift 2, and if the value is missing print a clear
error and exit non‑zero; keep references to the identity assignment
(identity="$2") and the shift 2 call so you modify that exact block.
- Around line 24-26: The pipeline that resolves recipients via "nix eval --file
${keysFile} roles.infra ..." must be validated before attempting to encrypt:
capture the output of that command into a variable (used by encryptState), check
that it succeeded and is non-empty, and if it failed or produced no recipients,
print an error and exit non-zero instead of piping to rage; update the logic
around encryptState/tfState to only call rage when the recipients string is
non-empty and to propagate nix eval errors (e.g., check its exit status or test
-n "$recipients"), referencing the existing symbols: nix eval (roles.infra),
${keysFile}, ${tfState}, and the encrypt step that invokes rage.
- Around line 34-37: encryptVars currently pipes data into rage without
validating that the source file (${tfVars}) exists or that recipients from
${keysFile} are valid; update the encryptVars block to first check that
${tfVars} exists and is non-empty and that the recipient list parsed from
${keysFile} (roles.infra) yields at least one valid recipient, failing with a
clear error message via builtins.abort or nix's builtins.trace if checks fail,
then proceed to run the nix eval | rage pipeline only when validations pass;
reference the symbols encryptVars, keysFile, roles.infra and ${tfVars} when
adding these guards so the logic is easy to locate.
In `@modules/acme.nix`:
- Around line 10-13: The email option is currently required even when ACME is
disabled; change the omnix.acme.email option to be nullable and only required
when omnix.acme.enable is true by setting its type to a nullable string (e.g.,
lib.types.nullOr lib.types.str) and providing a default null, then add a
validation/assertion in the module config that enforces config.omnix.acme.email
is non-null only if config.omnix.acme.enable is true (apply the same pattern to
the other options referenced around lines 16-21).
In `@modules/storage.nix`:
- Around line 10-13: The option volumeName must be constrained to a safe
character set before it’s used in the device path; update the volumeName
mkOption to include a validate function that rejects any value not matching a
safe regex (e.g. ^[A-Za-z0-9._-]+$) using builtins.match and throw a clear error
message, and keep using the validated volumeName where the device path is
interpolated so only allowed characters can reach that path construction.
In `@README.md`:
- Line 42: Add a short "Migration" subsection explaining how to replace
lib.mkRemote usage with lib.mkGitHooks: show a minimal before snippet using
mkRemote and the equivalent after snippet using mkGitHooks, list any changed or
removed arguments and their new names or defaults (e.g., optionX -> optionY,
default behavior changes), and include a note about optional rustfmt behavior if
relevant; place this note near the table entry for lib.mkGitHooks so users see
the concrete before/after example and argument/default changes when scanning the
docs.
In `@templates/do-service/.github/workflows/ci.yml`:
- Around line 52-53: The HOST_KEY evaluation is unguarded and can hard-fail
deployment if `nix eval --raw --file keys.nix keys.host` fails; change the step
to capture the command into a variable (e.g., HOST_KEY=$(nix eval --raw --file
keys.nix keys.host 2>/dev/null) or using set +e and capture exit code), verify
the command succeeded and HOST_KEY is non-empty before echoing "$HOST_IP
$HOST_KEY" >> ~/.ssh/known_hosts, and on failure emit a warning log and skip
adding the host instead of letting the job exit (use conditional checks on the
exit code or empty string and avoid unhandled failures).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9f60369a-a789-4430-9e35-5a52cff3f7a3
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
.github/workflows/ci.yml.github/workflows/integration-test.ymlREADME.mdROADMAP.mdflake.nixlib/bootstrap.nixlib/default.nixlib/deploy.nixlib/hooks.nixlib/remote.nixlib/shell.nixlib/terraform.nixmodules/acme.nixmodules/base.nixmodules/digitalocean.nixmodules/disko.nixmodules/firewall.nixmodules/services.nixmodules/storage.nixtemplates/do-service/.github/workflows/ci.ymltemplates/do-service/.github/workflows/integration-test.ymltemplates/do-service/config/secrets.nixtemplates/do-service/flake.nixtemplates/do-service/infra/secrets.nixtemplates/do-service/infra/terraform.tfvars.exampletemplates/do-service/infra/variables.tftemplates/do-service/keys.nixtemplates/do-service/os.nixtemplates/do-service/services.nix
💤 Files with no reviewable changes (1)
- lib/remote.nix
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
*
⚙️ CodeRabbit configuration file
Focus on providing constructive criticism. Whenever you see a suboptimal approach, suggest more idiomatic or robust alternative(s). Flag potential footguns. Suggest FP alternatives to mutable/imperative code. Point out architectural flaws like leaky abstractions, tight coupling, wrong level of abstraction, poor type modeling, over-abstraction, unclear domain boundaries. Code should generally be organized based on business concerns rather than technical aspects - suggest improvements if you find violations. Point out gaps in test coverage but suggest tests that are not too coupled to the implementation and actually test domain invariants and business logic
Files:
README.mdflake.nixROADMAP.md
**/*.md
⚙️ CodeRabbit configuration file
Focus on the contents of the docs and not on cosmetic things like markdown formatting. We use markdown files for various docs including but not limited to the north star system specification, SPEC.md, the plan for how to get there, ROADMAP.md, guidelines for AI contributors, AGENTS.md, project overview and instructions for human contributors, README.md. Think about the target audience of a document when deciding what comment to leave. For specifications and designs, suggest potential product, architectural, and UI/UX improvements. For plans, suggest changes that would make things more parallelizable and deliverable-focused. For instructions, suggest better rules and guidelines and point out missing instructions. In all cases, flag needless bloat, prefer clear concise writing, and consider the structure of the document and order of the sections
Files:
README.mdROADMAP.md
🔇 Additional comments (15)
modules/storage.nix (1)
3-6: Formatting-onlylet/inrefactor looks good.This improves readability without changing behavior.
templates/do-service/infra/terraform.tfvars.example (1)
1-2: Good guardrail for secret handling in templates.The new guidance clearly steers users away from creating plaintext tfvars manually and toward the encrypted workflow.
templates/do-service/keys.nix (1)
3-5: Formatting refactor is clean and behavior-preserving.This keeps the same keys/role mapping while improving readability.
Also applies to: 9-20
templates/do-service/config/secrets.nix (1)
1-4: Looks good — formatting-only normalization.No behavioral change introduced.
templates/do-service/infra/secrets.nix (1)
1-6: Reformat is safe and preserves secret key mapping semantics.lib/deploy.nix (1)
9-14: Nice refactor:targetSystemis threaded consistently through deploy activation.This improves portability while keeping wrapper behavior intact.
Also applies to: 50-61, 65-76, 93-98
templates/do-service/services.nix (1)
4-4: Good template default for per-service data isolation.Using
/mnt/data/my-serviceis clearer and safer than a generic shared path.modules/digitalocean.nix (1)
12-16: Good clarification on import/evaluation behavior.The comment clearly explains module import side effects vs runtime gating and makes integration intent easier to follow.
flake.nix (3)
97-104: Dev shell packages are still manually maintained.The packages list duplicates what's configured in
lib/hooks.nix. If hooks or tools change there, this list won't update automatically.
19-21: LGTM on git-hooks integration.Clean addition of the git-hooks input with proper nixpkgs follows.
46-59: Clear module layering pattern.Good approach: importing both the upstream
disko.nixosModules.diskofor core functionality and the localself.nixosModules.diskowrapper for omnix-specific assertions/configuration. The comment on line 46 clarifies intent.ROADMAP.md (2)
111-114:mkIntegrationTestdeliverable still lacks signature/usage specification.The description mentions parameterization by "project-specific config" but doesn't specify the function's return type or invocation pattern. Adding a brief expected signature would make this actionable.
40-81: Age-based secret management section is well-structured.Clear problem statement, explicit workflow requirements, parallelizable dependency graph, and concrete deliverables. The "Optional:" label on secretspec integration addresses prior ambiguity.
templates/do-service/flake.nix (2)
95-105: LGTM on theresolveIppackage.Good extraction into a standalone shell application. The implementation correctly sources the shared helper from
infraPkgs.resolveIpand outputs the result. TheruntimeInputsproperly declaresrageandjqdependencies.
27-30: Clean module simplification.Reducing to just
omnix.nixosModules.defaultplus the local./os.nixis the right call—the default module already pulls in all upstream and omnix modules (disko, ragenix, and all others per the parent flake's definition).
| - name: Teardown | ||
| if: always() | ||
| working-directory: ${{ env.TEST_DIR }} | ||
| run: nix run .#tfDestroy -- -i ~/.ssh/id_ed25519 || true |
There was a problem hiding this comment.
Don't suppress cleanup failures.
if: always() already ensures teardown runs. Keeping || true here means the job can finish green while the droplet is still alive and accruing cost.
Suggested change
- run: nix run .#tfDestroy -- -i ~/.ssh/id_ed25519 || true
+ run: nix run .#tfDestroy -- -i ~/.ssh/id_ed25519📝 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.
| - name: Teardown | |
| if: always() | |
| working-directory: ${{ env.TEST_DIR }} | |
| run: nix run .#tfDestroy -- -i ~/.ssh/id_ed25519 || true | |
| - name: Teardown | |
| if: always() | |
| working-directory: ${{ env.TEST_DIR }} | |
| run: nix run .#tfDestroy -- -i ~/.ssh/id_ed25519 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/integration-test.yml around lines 131 - 134, The teardown
step currently silences failures by appending "|| true" to the run command,
which can hide cleanup errors; edit the Teardown job's run command (the line
invoking "nix run .#tfDestroy -- -i ~/.ssh/id_ed25519") to remove "|| true" so
failures in tfDestroy propagate and cause the job to fail (since the job already
uses "if: always()"), ensuring cleanup errors are visible and not ignored.
| assertions = [ | ||
| { | ||
| assertion = builtins.hasAttr "disko" config; | ||
| message = "omnix.disko requires the upstream disko NixOS module. Import disko.nixosModules.disko or use omnix.nixosModules.default which includes it."; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In NixOS modules, what is the recommended way to check if an upstream option namespace exists: builtins.hasAttr "disko" configoroptions ? disko?
💡 Result:
In NixOS modules, the recommended way to check if an upstream option namespace exists is options ? disko. This checks the options argument (the merged option declarations from all modules), which is available early during evaluation and safe to use with the ? operator or lib.hasAttr options "disko". It determines if any module has declared options.disko. In contrast, builtins.hasAttr "disko" config checks the config argument (merged option values), which may not have the attribute even if declared (e.g., if unset or lazy), and can cause infinite recursion if used in certain contexts like lib.optionalAttrs. Official NixOS manual confirms options is provided to modules for option declarations. NixOS Discourse threads recommend options ? foo for detecting option availability across versions or conditionally, avoiding evaluation errors if undeclared.
Citations:
- 1: https://discourse.nixos.org/t/how-to-check-is-an-option-available/2919/3
- 2: https://nixos.org/manual/nixos/unstable/options
- 3: https://discourse.nixos.org/t/understanding-top-level-arguments-in-configuration-nix/68993
Use options ? disko for the import guard, not builtins.hasAttr "disko" config.
This guard is fragile because it checks merged runtime config values, not option declarations. Use options ? disko to safely detect if the option namespace has been declared, which is the idiomatic approach in NixOS modules for module-import validation.
♻️ Suggested guard pattern
-{ lib, config, ... }:
+{ lib, config, options, ... }:
@@
- assertions = [
- {
- assertion = builtins.hasAttr "disko" config;
- message = "omnix.disko requires the upstream disko NixOS module. Import disko.nixosModules.disko or use omnix.nixosModules.default which includes it.";
- }
- ];
+ assertions = [
+ {
+ assertion = options ? disko;
+ message = "omnix.disko requires the upstream disko NixOS module. Import disko.nixosModules.disko or use omnix.nixosModules.default which includes it.";
+ }
+ ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/disko.nix` around lines 17 - 21, The assertion currently uses
builtins.hasAttr "disko" config inside the assertions block; replace that
runtime-config check with an options namespace test by using the idiomatic guard
options ? disko so the module detects the disko option declarations rather than
merged config values—update the assertion expression (the entry named assertion
in the assertions list) to use options ? disko and keep the existing message
text.
| - name: Teardown | ||
| if: always() | ||
| run: nix run .#tfDestroy -- -i ~/.ssh/id_ed25519 || true |
There was a problem hiding this comment.
Don't hide destroy failures.
if: always() already guarantees teardown runs. The trailing || true can leave droplets around while the workflow still reports green.
Suggested change
- run: nix run .#tfDestroy -- -i ~/.ssh/id_ed25519 || true
+ run: nix run .#tfDestroy -- -i ~/.ssh/id_ed25519📝 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.
| - name: Teardown | |
| if: always() | |
| run: nix run .#tfDestroy -- -i ~/.ssh/id_ed25519 || true | |
| - name: Teardown | |
| if: always() | |
| run: nix run .#tfDestroy -- -i ~/.ssh/id_ed25519 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/do-service/.github/workflows/integration-test.yml` around lines 89
- 91, The Teardown step ("Teardown") masks failures by appending "|| true" to
the run command; remove the trailing "|| true" from the "nix run .#tfDestroy --
-i ~/.ssh/id_ed25519 || true" invocation so the step fails on errors (keep the
existing "if: always()" to ensure it still runs), allowing workflow failure when
destroy fails and preventing orphaned droplets.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/services.nix (1)
88-91: 🧹 Nitpick | 🔵 TrivialConsider using
lib.types.strforconfigDir.The coding guidelines prefer
lib.types.stroverlib.types.pathfor values that may come from terraform or other dynamic sources. For consistency withdataDirandlogDir(which already uselib.types.str), and to avoid type coercion issues when the config directory path is provided dynamically, consider changing this type.♻️ Proposed fix
configDir = lib.mkOption { - type = lib.types.path; + type = lib.types.str; description = "Path to directory containing <service-name>.toml configs"; };As per coding guidelines: "Prefer
lib.types.stroverlib.types.pathfor values that may come from terraform or other dynamic sources".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/services.nix` around lines 88 - 91, Change the configDir option definition to use lib.types.str instead of lib.types.path: update the lib.mkOption for configDir so its type matches dataDir and logDir (lib.types.str) to avoid path-type coercion when values come from terraform or other dynamic sources; keep the existing description and option name (configDir) unchanged.
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
16-19:⚠️ Potential issue | 🟡 MinorUpdate action versions to the latest stable releases.
The workflow pins
nix-installer-action@v17andmagic-nix-cache-action@v8, but these are outdated. Update tonix-installer-action@v21andmagic-nix-cache-action@v9for the latest bug fixes and improvements.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 16 - 19, Update the pinned action versions in the CI workflow: replace DeterminateSystems/nix-installer-action@v17 with DeterminateSystems/nix-installer-action@v21 and DeterminateSystems/magic-nix-cache-action@v8 with DeterminateSystems/magic-nix-cache-action@v9 in the steps that currently reference those actions (lines showing actions/checkout@v4, DeterminateSystems/nix-installer-action@v17, DeterminateSystems/magic-nix-cache-action@v8, and the nix flake check step); ensure no other steps rely on the old versions and run the workflow to verify nix flake check still passes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/integration-test.yml:
- Around line 71-84: Add an inline comment next to the generated host entry in
the keys.nix content (the "host" key assignment inside the rec { keys = { ... }
} block) stating that the ssh-ed25519 "AAAAC3NzaC1lZDI1NTE5AAAAIPLACEHOLDER"
value is an intentional scaffold placeholder and that the real host key should
be retrieved and replaced post‑provisioning (or via provisioning scripts) for
permanent deployments; keep the placeholder behavior unchanged but make the
intent explicit for future readers.
---
Outside diff comments:
In `@modules/services.nix`:
- Around line 88-91: Change the configDir option definition to use lib.types.str
instead of lib.types.path: update the lib.mkOption for configDir so its type
matches dataDir and logDir (lib.types.str) to avoid path-type coercion when
values come from terraform or other dynamic sources; keep the existing
description and option name (configDir) unchanged.
---
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 16-19: Update the pinned action versions in the CI workflow:
replace DeterminateSystems/nix-installer-action@v17 with
DeterminateSystems/nix-installer-action@v21 and
DeterminateSystems/magic-nix-cache-action@v8 with
DeterminateSystems/magic-nix-cache-action@v9 in the steps that currently
reference those actions (lines showing actions/checkout@v4,
DeterminateSystems/nix-installer-action@v17,
DeterminateSystems/magic-nix-cache-action@v8, and the nix flake check step);
ensure no other steps rely on the old versions and run the workflow to verify
nix flake check still passes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b7d1f5b9-b2f5-48f4-8e7b-6964b4d5e861
📒 Files selected for processing (9)
.github/workflows/ci.yml.github/workflows/integration-test.ymlROADMAP.mdmodules/acme.nixmodules/services.nixtemplates/do-service/.github/workflows/ci.ymltemplates/do-service/.github/workflows/integration-test.ymltemplates/do-service/flake.nixtemplates/do-service/infra/variables.tf
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Flake check
🧰 Additional context used
📓 Path-based instructions (3)
**/*.nix
📄 CodeRabbit inference engine (AGENTS.md)
**/*.nix: Uselib.mkOptionwith proper types for all module options
Every module must be independently usable (no implicit dependencies on other omnix modules)
Options must live under theomnix.*namespace
Uselib.mkIf cfg.enablefor conditional configuration
Uselib.mkDefaultfor overridable defaults
Every configurable value must be a module option with a type and description
Preferlib.types.stroverlib.types.pathfor values that may come from terraform or other dynamic sources
Group related options under a singleomnix.<module>prefix
Functions take an attrset of required parameters
Return an attrset of derivations or values from library functions
Usepkgs.writeShellApplicationfor shell scripts (not rawwriteScript)
IncluderuntimeInputsin shell scripts and never assume tools are on PATH
All shell scripts must useset -eo pipefail
When changing module option interfaces, preserve the old option withlib.mkRenamedOptionModuleorlib.mkRemovedOptionModulewith a clear migration message
Files:
modules/acme.nixmodules/services.nixtemplates/do-service/flake.nix
*
⚙️ CodeRabbit configuration file
Focus on providing constructive criticism. Whenever you see a suboptimal approach, suggest more idiomatic or robust alternative(s). Flag potential footguns. Suggest FP alternatives to mutable/imperative code. Point out architectural flaws like leaky abstractions, tight coupling, wrong level of abstraction, poor type modeling, over-abstraction, unclear domain boundaries. Code should generally be organized based on business concerns rather than technical aspects - suggest improvements if you find violations. Point out gaps in test coverage but suggest tests that are not too coupled to the implementation and actually test domain invariants and business logic
Files:
ROADMAP.md
**/*.md
⚙️ CodeRabbit configuration file
Focus on the contents of the docs and not on cosmetic things like markdown formatting. We use markdown files for various docs including but not limited to the north star system specification, SPEC.md, the plan for how to get there, ROADMAP.md, guidelines for AI contributors, AGENTS.md, project overview and instructions for human contributors, README.md. Think about the target audience of a document when deciding what comment to leave. For specifications and designs, suggest potential product, architectural, and UI/UX improvements. For plans, suggest changes that would make things more parallelizable and deliverable-focused. For instructions, suggest better rules and guidelines and point out missing instructions. In all cases, flag needless bloat, prefer clear concise writing, and consider the structure of the document and order of the sections
Files:
ROADMAP.md
🧠 Learnings (14)
📓 Common learnings
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.571Z
Learning: Applies to **/*.nix : Use `pkgs.writeShellApplication` for shell scripts (not raw `writeScript`)
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.571Z
Learning: Applies to **/*.nix : Include `runtimeInputs` in shell scripts and never assume tools are on PATH
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.571Z
Learning: Template projects should pass `nix flake check`
📚 Learning: 2026-03-21T15:47:14.571Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.571Z
Learning: Applies to **/*.nix : Group related options under a single `omnix.<module>` prefix
Applied to files:
modules/acme.nixROADMAP.mdmodules/services.nixtemplates/do-service/flake.nix
📚 Learning: 2026-03-21T15:47:14.571Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.571Z
Learning: Applies to **/*.nix : Options must live under the `omnix.*` namespace
Applied to files:
modules/acme.nixtemplates/do-service/flake.nix
📚 Learning: 2026-03-21T15:47:14.571Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.571Z
Learning: Applies to **/*.nix : Every module must be independently usable (no implicit dependencies on other omnix modules)
Applied to files:
modules/acme.nixROADMAP.mdtemplates/do-service/flake.nix
📚 Learning: 2026-03-21T15:47:14.571Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.571Z
Learning: Applies to **/*.nix : Use `lib.mkOption` with proper types for all module options
Applied to files:
modules/acme.nixmodules/services.nix
📚 Learning: 2026-03-21T15:47:14.571Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.571Z
Learning: Applies to **/*.nix : Every configurable value must be a module option with a type and description
Applied to files:
modules/acme.nix
📚 Learning: 2026-03-21T15:47:14.571Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.571Z
Learning: Applies to **/*.nix : When changing module option interfaces, preserve the old option with `lib.mkRenamedOptionModule` or `lib.mkRemovedOptionModule` with a clear migration message
Applied to files:
modules/acme.nixROADMAP.mdmodules/services.nix
📚 Learning: 2026-03-21T15:47:14.571Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.571Z
Learning: Template projects should pass `nix flake check`
Applied to files:
.github/workflows/ci.ymlROADMAP.mdtemplates/do-service/flake.nix
📚 Learning: 2026-03-21T15:47:14.571Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.571Z
Learning: Never suppress Nix evaluation errors or warnings
Applied to files:
templates/do-service/.github/workflows/ci.yml
📚 Learning: 2026-03-21T15:47:14.571Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.571Z
Learning: Applies to **/*.nix : Use `lib.mkIf cfg.enable` for conditional configuration
Applied to files:
modules/services.nix
📚 Learning: 2026-03-21T15:47:14.571Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.571Z
Learning: Applies to **/*.nix : Functions take an attrset of required parameters
Applied to files:
modules/services.nixtemplates/do-service/flake.nix
📚 Learning: 2026-03-21T15:47:14.571Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.571Z
Learning: Applies to **/*.nix : Include `runtimeInputs` in shell scripts and never assume tools are on PATH
Applied to files:
templates/do-service/flake.nix
📚 Learning: 2026-03-21T15:47:14.571Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.571Z
Learning: Applies to **/*.nix : Use `lib.mkDefault` for overridable defaults
Applied to files:
templates/do-service/flake.nix
📚 Learning: 2026-03-21T15:47:14.571Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.571Z
Learning: Applies to **/*.nix : Use `pkgs.writeShellApplication` for shell scripts (not raw `writeScript`)
Applied to files:
templates/do-service/flake.nix
🔇 Additional comments (25)
modules/acme.nix (3)
1-5: LGTM!Standard NixOS module header with proper argument destructuring and cfg alias pattern.
7-15: LGTM! Previous feedback addressed.The
lib.types.nullOr lib.types.strwithdefault = null, allowing the module to evaluate cleanly whenenable = false. Both options have proper types and descriptions under theomnix.acmenamespace. As per coding guidelines: "Options must live under theomnix.*namespace" and "Every configurable value must be a module option with a type and description."
17-29: LGTM! Well-structured conditional configuration.The implementation correctly:
- Uses
lib.mkIf cfg.enablefor conditional configuration- Includes an assertion ensuring
- Sets required ACME configuration (
acceptTerms,defaults.email)The module is independently usable with no implicit dependencies on other omnix modules. As per coding guidelines: "Use
lib.mkIf cfg.enablefor conditional configuration" and "Every module must be independently usable."modules/services.nix (5)
8-27: LGTM on the function structure.The curried function signature
name: svcCfg:is appropriate for use withlib.mapAttrson line 132. The internal let bindings are well-organized.Note: The
ExecStartstring concatenation concern was previously flagged and remains unaddressed.
147-163: Owner/group duplication in logrotate settings.Lines 152-154 still compute owner/group inline, duplicating the logic already extracted at lines 142-143 in the tmpfiles block. This was previously flagged.
172-177: Group creation logic comment suggestion still applicable.The complex conditional
(cfg.user != null && !cfg.dynamicUser || cfg.group != null)with nestedoptionalAttrshandles multiple scenarios. A brief inline comment would help maintainers understand the intent. This was previously suggested.
45-59: LGTM on the serviceConfig refactoring.The use of
lib.optionalAttrsis idiomatic, andlogDiris now correctly included inReadWritePathsalongsidedataDir, addressing the previous access concern for dynamic users.
134-145: LGTM on tmpfiles refactoring.The extraction of
ownerandgroupto shared let bindings (lines 142-143) is clean and addresses the previous duplication concern for this section.templates/do-service/.github/workflows/ci.yml (2)
54-57: Defensive handling for HOST_KEY is implemented but silently fails.The
nix evalfor HOST_KEY now has error suppression and an empty check, which prevents workflow abort on malformedkeys.nix. However, ifHOST_KEYis empty whileHOST_IPis set, the workflow continues silently without adding the host toknown_hosts, potentially causing SSH verification failures later.Consider logging a warning when HOST_KEY is empty to aid debugging:
🛡️ Suggested warning
HOST_KEY=$(nix eval --raw --file keys.nix keys.host 2>/dev/null || echo "") if [ -n "$HOST_KEY" ]; then echo "$HOST_IP $HOST_KEY" >> ~/.ssh/known_hosts + else + echo "Warning: Could not read host key from keys.nix" >&2 fi
35-37: LGTM!The
id-token: writepermission is correctly scoped to the deploy job only, following the principle of least privilege. Jobs that don't need OIDC capabilities no longer have elevated permissions.templates/do-service/.github/workflows/integration-test.yml (2)
92-94: Don't suppress teardown failures — this can leave orphaned resources.The
|| trueon line 94 maskstfDestroyerrors, meaning the workflow can report success while the droplet remains running and accruing cost. Sinceif: always()already ensures this step runs regardless of prior failures, the error suppression serves no purpose and hides critical cleanup issues.🔧 Suggested fix
- name: Teardown if: always() - run: nix run .#tfDestroy -- -i ~/.ssh/id_ed25519 || true + run: nix run .#tfDestroy -- -i ~/.ssh/id_ed25519
28-30: LGTM!The workflow inputs are now correctly wired to Terraform via
TF_VAR_droplet_sizeandTF_VAR_regionenvironment variables at the job level. This ensures the UI-provided values are actually used during infrastructure provisioning.ROADMAP.md (3)
111-113: Clarify themkIntegrationTestdeliverable with expected signature.The description mentions parameterization by project-specific config but doesn't specify what the function returns or how consumers invoke it. For a roadmap, adding the expected signature or a brief usage example would help contributors understand the scope.
3-21: LGTM!Good use of a collapsible
<details>block to keep the roadmap focused on forward-looking work while preserving the completed PR review findings for reference.
52-64: LGTM!The Mermaid diagram now correctly shows parallel development paths: the NixOS module (F) and deploy-rs activation (G) can be developed in parallel since they both depend only on the age encrypt/decrypt core (B), not on the CLI. This better reflects the actual dependency structure.
.github/workflows/integration-test.yml (3)
133-136: Don't suppress teardown failures — orphaned droplets accrue cost.The
|| truemaskstfDestroyerrors, allowing the workflow to succeed while leaving the droplet running. Sinceif: always()already ensures this step executes regardless of prior failures, the error suppression only hides cleanup issues.🔧 Suggested fix
- name: Teardown if: always() working-directory: ${{ env.TEST_DIR }} - run: nix run .#tfDestroy -- -i ~/.ssh/id_ed25519 || true + run: nix run .#tfDestroy -- -i ~/.ssh/id_ed25519
100-111: Consider addingBatchMode=yesto the SSH retry loop.Adding
BatchMode=yesprevents the SSH client from hanging on unexpected password prompts or interactive authentication, making the retry loop more deterministic in CI environments.🔧 Suggested fix
- name: Wait for SSH run: | retries=0 - until ssh -o StrictHostKeyChecking=no -o ConnectTimeout=5 \ + until ssh -o StrictHostKeyChecking=no -o ConnectTimeout=5 -o BatchMode=yes \ -i ~/.ssh/id_ed25519 "root@$HOST_IP" true 2>/dev/null; do
31-33: LGTM!The workflow dispatch inputs are correctly wired to Terraform via
TF_VAR_droplet_sizeandTF_VAR_regionenvironment variables, ensuring UI-provided values are used during provisioning..github/workflows/ci.yml (2)
8-9: LGTM!The permissions are correctly scoped to
contents: readonly. This follows the principle of least privilege for a workflow that only needs to read the repository and runnix flake check.
21-39: LGTM!The examples job correctly validates NixOS configurations using
nix evalto check the derivation path exists andnix build --dry-runto verify the configuration can be built. The matrix strategy efficiently tests all example configurations in parallel.templates/do-service/infra/variables.tf (2)
5-10: LGTM!The
do_tokenvalidation now correctly rejects empty and whitespace-only values usingtrimspace(), andnullable = falseensures the variable must be explicitly set. The error message is clear and actionable.
35-39: LGTM!The
volume_size_gbvalidation now correctly enforces integer values by comparing againstfloor(), preventing fractional GB inputs that wouldn't make sense for volume sizing.templates/do-service/flake.nix (3)
27-30: LGTM!The module list is correctly simplified to use
omnix.nixosModules.default, which aggregates all required modules (disko, ragenix, digitalocean, base, storage, services, firewall, acme) as shown in the rootflake.nix. This reduces duplication and ensures the template automatically inherits any new modules added to the default set.
95-105: LGTM!The
resolveIppackage correctly usespkgs.writeShellApplicationwith explicitruntimeInputsforrageandjq, following the coding guidelines. The package properly wraps the shell snippet frominfraPkgs.resolveIpand outputs the resolvedhost_ipvariable. Based on learnings: "Usepkgs.writeShellApplicationfor shell scripts (not rawwriteScript)" and "IncluderuntimeInputsin shell scripts and never assume tools are on PATH".
33-44: LGTM!The comment clearly explains why the deploy configuration is hardcoded to
x86_64-linux— the remote NixOS host is always x86_64-linux regardless of the local development machine's architecture. This prevents confusion when developers on ARM Macs see the hardcoded system.
| cat > keys.nix <<KEYS | ||
| rec { | ||
| keys = { | ||
| operator = "$PUBKEY"; | ||
| ci = "$PUBKEY"; | ||
| host = "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIPLACEHOLDER"; | ||
| }; | ||
| roles = with keys; { | ||
| infra = [ operator ci ]; | ||
| service = [ host operator ]; | ||
| ssh = [ operator ci ]; | ||
| }; | ||
| } | ||
| KEYS |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Placeholder host key in generated keys.nix.
The dynamically generated keys.nix uses a placeholder value "ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIPLACEHOLDER" for the host key. This is acceptable since the actual host doesn't exist during scaffold time, but consider adding a comment explaining this is expected and that the real host key would typically be retrieved post-provisioning for permanent deployments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/integration-test.yml around lines 71 - 84, Add an inline
comment next to the generated host entry in the keys.nix content (the "host" key
assignment inside the rec { keys = { ... } } block) stating that the ssh-ed25519
"AAAAC3NzaC1lZDI1NTE5AAAAIPLACEHOLDER" value is an intentional scaffold
placeholder and that the real host key should be retrieved and replaced
post‑provisioning (or via provisioning scripts) for permanent deployments; keep
the placeholder behavior unchanged but make the intent explicit for future
readers.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/base.nix (1)
26-29:⚠️ Potential issue | 🟠 MajorThis is a breaking interface change without a migration path.
Removing the
stateVersiondefault makes previously valid configs fail whenomnix.base.enable = true. Keep a compatibility default (or add an explicit migration mechanism/message) before enforcing mandatory configuration.🛠️ Backward-compatible fix
stateVersion = lib.mkOption { type = lib.types.str; + default = "24.11"; description = "NixOS state version"; };As per coding guidelines,
When changing module option interfaces, preserve the old option with lib.mkRenamedOptionModule or lib.mkRemovedOptionModule with a clear migration message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/base.nix` around lines 26 - 29, The change removed the default for the NixOS module option stateVersion (declared via lib.mkOption), causing a breaking interface when omnix.base.enable = true; restore backward compatibility by reintroducing the old option with a compatibility wrapper: either add a default value for stateVersion again or replace the removed option with lib.mkRenamedOptionModule / lib.mkRemovedOptionModule (referencing stateVersion and the module where stateVersion was defined) and provide a clear migration message explaining the new required configuration and how to set the stateVersion so existing configs continue to work.
♻️ Duplicate comments (6)
modules/services.nix (2)
14-25:⚠️ Potential issue | 🟠 MajorUse a list for
ExecStartto safely handle arguments with special characters.The
builtins.concatStringsSep " "approach doesn't escape arguments containing whitespace or quotes. IfextraArgscontains["--name" "my service"], systemd will incorrectly split"my service"into two arguments.Assign
ExecStartas a list instead:- execStart = - if svcCfg.extraArgs == [ ] then - "${path} --config ${configFile}" - else - builtins.concatStringsSep " " ( - [ - path - "--config" - "${configFile}" - ] - ++ svcCfg.extraArgs - ); + execStart = [ path "--config" configFile ] ++ svcCfg.extraArgs;NixOS will automatically escape each element when generating the unit file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/services.nix` around lines 14 - 25, The ExecStart currently builds a shell-escaped string using builtins.concatStringsSep which breaks when elements in svcCfg.extraArgs contain spaces/quotes; change execStart to be a Nix list so systemd receives properly escaped argv elements: construct execStart as a list starting with path, "--config", "${configFile}" and then append svcCfg.extraArgs (instead of concatenating into a single string), and ensure the empty-extraArgs case returns a list too (no concatStringsSep or manual string joining).
134-145: 🧹 Nitpick | 🔵 TrivialConsider extracting shared
owner/groupbindings to reduce duplication.The owner/group fallback logic appears in two places:
- Lines 142-143 for tmpfiles rules
- Lines 152-153 for logrotate
sudirectiveA single
letblock scoping both would keep them in sync:+ config = lib.mkIf (cfg.definitions != { }) ( + let + owner = if cfg.user != null then cfg.user else "root"; + group = if cfg.group != null then cfg.group else "root"; + in + { # ... use owner/group in both tmpfiles and logrotate + } + );Also applies to: 147-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/services.nix` around lines 134 - 145, The owner/group fallback logic is duplicated; extract the bindings for owner and group into a single shared let so both systemd.tmpfiles.rules and the logrotate su directive reuse the same values. Move the owner = if cfg.user != null then cfg.user else "root" and group = if cfg.group != null then cfg.group else "root" definitions into a surrounding let that encloses both the systemd.tmpfiles.rules expression and the logrotate configuration, then reference those owner and group symbols from systemd.tmpfiles.rules (the map producing "d ${dir} 0770 ${owner} ${group} -") and from the logrotate su directive to keep them in sync.README.md (1)
42-42:⚠️ Potential issue | 🟡 MinorAdd a short migration note for
mkRemote→lib.mkGitHooks.Line 42 documents the new function but not the upgrade path. Add a minimal before/after snippet so existing users can migrate quickly.
As per coding guidelines,
**/*.md: “point out missing instructions” and tailor docs to the target audience.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 42, Add a short migration note under the `lib.mkGitHooks` entry that shows a minimal before/after snippet for users migrating from `mkRemote` to `lib.mkGitHooks`; mention the original call using `mkRemote` and the new equivalent using `lib.mkGitHooks`, include any required option name changes (e.g., hook list, formats) and a one-line instruction that they should replace `mkRemote(...)` with `lib.mkGitHooks(...)` to make the upgrade path explicit for existing users.modules/disko.nix (1)
19-20:⚠️ Potential issue | 🟠 MajorUse option-namespace detection for the upstream module guard.
Line 19 should check declared options (
options ? disko) rather than merged runtime config (builtins.hasAttr "disko" config) to avoid brittle evaluation behavior.♻️ Proposed fix
-{ lib, config, ... }: +{ lib, config, options, ... }: @@ - assertion = builtins.hasAttr "disko" config; + assertion = options ? disko;#!/bin/bash set -euo pipefail # Verify current guard expression and whether options-based guard is already used. rg -nP --type=nix -C3 'builtins\.hasAttr\s+"disko"\s+config|options\s*\?\s*disko'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/disko.nix` around lines 19 - 20, The module guard currently uses the runtime check builtins.hasAttr "disko" config in the assertion; replace that with an options-level detection (options ? disko) so the assertion uses declared options rather than merged runtime config. Update the assertion attribute (named assertion) to use options ? disko and keep the existing message unchanged; ensure the new expression is used where the current builtins.hasAttr "disko" config appears (e.g., the assertion binding) so the guard is evaluated against declared options.lib/bootstrap.nix (1)
18-25:⚠️ Potential issue | 🟠 MajorDeclare all invoked shell tools in
runtimeInputs.Line 49 uses
awkand Line 56 usesgrep, but neither tool is declared inruntimeInputs. This can break execution in clean environments.🧩 Proposed fix
runtimeInputs = [ pkgs.rage pkgs.jq + pkgs.gawk + pkgs.gnugrep pkgs.gnused pkgs.openssh nixos-anywhere.packages.${system}.default ]Based on learnings,
Include runtimeInputs in shell scripts and never assume tools are on PATH.Also applies to: 49-56
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/bootstrap.nix` around lines 18 - 25, The runtimeInputs list is missing the shell tools used by the scripts (awk and grep), which breaks clean builds; update the runtimeInputs array (the existing runtimeInputs variable that currently includes pkgs.rage, pkgs.jq, pkgs.gnused, pkgs.openssh, nixos-anywhere.packages.${system}.default, and optional ragenixPkg) to also include pkgs.gawk and pkgs.gnugrep (or the distro-appropriate awk/grep packages) so the calls that invoke awk and grep in the shell scripts will be available at runtime..github/workflows/integration-test.yml (1)
100-111: 🧹 Nitpick | 🔵 TrivialConsider adding
BatchMode=yesto SSH options.Adding
BatchMode=yesprevents the SSH client from hanging on unexpected password prompts, making the retry loop fail-fast in CI environments.Suggested fix
- name: Wait for SSH run: | retries=0 - until ssh -o StrictHostKeyChecking=no -o ConnectTimeout=5 \ + until ssh -o StrictHostKeyChecking=no -o ConnectTimeout=5 -o BatchMode=yes \ -i ~/.ssh/id_ed25519 "root@$HOST_IP" true 2>/dev/null; do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/integration-test.yml around lines 100 - 111, The "Wait for SSH" retry loop should add BatchMode=yes to the ssh command to avoid interactive password prompts; update the ssh invocation used in that step (the until ssh ... "root@$HOST_IP" true) to include -o BatchMode=yes alongside -o StrictHostKeyChecking=no and -o ConnectTimeout=5 so the loop fails fast in CI when a key is missing or authentication would prompt.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.coderabbit.yaml:
- Around line 32-34: The path_instructions entry currently uses path: "*" which
only matches top-level files; change the repository-wide glob in the
.coderabbit.yaml path_instructions block (the path_instructions key and its
path: "*" entry) to use a recursive pattern such as path: "/*" or an empty
string "" so the instructions apply to all nested files; update that path value
accordingly and keep the rest of the path_instructions configuration unchanged.
In @.github/workflows/ci.yml:
- Around line 17-18: The workflow pins third-party GitHub Actions using mutable
tags (actions/checkout@v4, DeterminateSystems/nix-installer-action@v17,
DeterminateSystems/magic-nix-cache-action@v8); replace each tag with the
corresponding full commit SHA for every usage of those actions in the file (both
occurrences), optionally keeping the tag as a trailing comment for readability,
so each uses the immutable ref (full SHA) instead of `@vX` to improve supply-chain
integrity and reproducibility.
In @.github/workflows/integration-test.yml:
- Around line 45-50: The workflow step "Setup SSH identity" uses the wrong
secret name; change the environment variable reference in the echo line from ${{
secrets.SSH_KEY }} to ${{ secrets.SSH_PRIVATE_KEY }} (and update any other
occurrences of secrets.SSH_KEY in this workflow) so it matches the template's
secret naming; ensure the step that writes ~/.ssh/id_ed25519 still uses the
updated secret reference and keep existing chmod/mkdir lines unchanged.
In `@modules/acme.nix`:
- Around line 25-27: security.acme.acceptTerms is hard-set to true which
prevents downstream overrides; change the assignment to use lib.mkDefault so the
default value can be overridden (e.g., replace the direct true with
lib.mkDefault true for security.acme.acceptTerms while keeping defaults.email =
cfg.email unchanged), referencing the security.acme.acceptTerms attribute and
lib.mkDefault helper.
In `@modules/firewall.nix`:
- Around line 13-14: Add an assertion in modules/acme.nix to require
omnix.firewall.enableHTTP when ACME is enabled: inside the existing assertions
attribute, add an assertion that checks cfg.enable implies
config.omnix.firewall.enableHTTP and provide a clear message like "omnix.acme
requires omnix.firewall.enableHTTP = true for HTTP-01 challenges"; reference the
assertions array and the cfg.enable/config.omnix.firewall.enableHTTP symbols so
the module will fail fast if ACME is turned on but firewall HTTP is disabled.
In `@templates/do-service/.github/workflows/ci.yml`:
- Around line 17-18: Replace mutable release tags with immutable commit SHAs for
all third-party GitHub Actions referenced (e.g., actions/checkout@v4,
DeterminateSystems/nix-installer-action@v17,
DeterminateSystems/magic-nix-cache-action@v8 and any other `@v` tags in the
workflow) by looking up the corresponding commit SHA on each action's GitHub
repo and updating the workflow references to use @<commit-sha>; ensure every
occurrence of the mutable tag is updated so no action remains pinned to a
mutable release.
In `@templates/do-service/.github/workflows/integration-test.yml`:
- Around line 61-72: In the "Wait for SSH" workflow step the ssh command can
hang on interactive prompts; update the ssh invocation (the until ssh ... true
loop) to include the option -o BatchMode=yes alongside -o
StrictHostKeyChecking=no and -o ConnectTimeout=5 so SSH exits immediately on
auth prompts and the retry loop remains deterministic in CI.
In `@templates/do-service/os.nix`:
- Line 11: Update the stateVersion value to match the repository's pinned NixOS
release: change the stateVersion declaration (symbol: stateVersion) from "24.11"
to "25.11" so the template aligns with the pinned/examples release and avoids
deployment drift.
---
Outside diff comments:
In `@modules/base.nix`:
- Around line 26-29: The change removed the default for the NixOS module option
stateVersion (declared via lib.mkOption), causing a breaking interface when
omnix.base.enable = true; restore backward compatibility by reintroducing the
old option with a compatibility wrapper: either add a default value for
stateVersion again or replace the removed option with lib.mkRenamedOptionModule
/ lib.mkRemovedOptionModule (referencing stateVersion and the module where
stateVersion was defined) and provide a clear migration message explaining the
new required configuration and how to set the stateVersion so existing configs
continue to work.
---
Duplicate comments:
In @.github/workflows/integration-test.yml:
- Around line 100-111: The "Wait for SSH" retry loop should add BatchMode=yes to
the ssh command to avoid interactive password prompts; update the ssh invocation
used in that step (the until ssh ... "root@$HOST_IP" true) to include -o
BatchMode=yes alongside -o StrictHostKeyChecking=no and -o ConnectTimeout=5 so
the loop fails fast in CI when a key is missing or authentication would prompt.
In `@lib/bootstrap.nix`:
- Around line 18-25: The runtimeInputs list is missing the shell tools used by
the scripts (awk and grep), which breaks clean builds; update the runtimeInputs
array (the existing runtimeInputs variable that currently includes pkgs.rage,
pkgs.jq, pkgs.gnused, pkgs.openssh, nixos-anywhere.packages.${system}.default,
and optional ragenixPkg) to also include pkgs.gawk and pkgs.gnugrep (or the
distro-appropriate awk/grep packages) so the calls that invoke awk and grep in
the shell scripts will be available at runtime.
In `@modules/disko.nix`:
- Around line 19-20: The module guard currently uses the runtime check
builtins.hasAttr "disko" config in the assertion; replace that with an
options-level detection (options ? disko) so the assertion uses declared options
rather than merged runtime config. Update the assertion attribute (named
assertion) to use options ? disko and keep the existing message unchanged;
ensure the new expression is used where the current builtins.hasAttr "disko"
config appears (e.g., the assertion binding) so the guard is evaluated against
declared options.
In `@modules/services.nix`:
- Around line 14-25: The ExecStart currently builds a shell-escaped string using
builtins.concatStringsSep which breaks when elements in svcCfg.extraArgs contain
spaces/quotes; change execStart to be a Nix list so systemd receives properly
escaped argv elements: construct execStart as a list starting with path,
"--config", "${configFile}" and then append svcCfg.extraArgs (instead of
concatenating into a single string), and ensure the empty-extraArgs case returns
a list too (no concatStringsSep or manual string joining).
- Around line 134-145: The owner/group fallback logic is duplicated; extract the
bindings for owner and group into a single shared let so both
systemd.tmpfiles.rules and the logrotate su directive reuse the same values.
Move the owner = if cfg.user != null then cfg.user else "root" and group = if
cfg.group != null then cfg.group else "root" definitions into a surrounding let
that encloses both the systemd.tmpfiles.rules expression and the logrotate
configuration, then reference those owner and group symbols from
systemd.tmpfiles.rules (the map producing "d ${dir} 0770 ${owner} ${group} -")
and from the logrotate su directive to keep them in sync.
In `@README.md`:
- Line 42: Add a short migration note under the `lib.mkGitHooks` entry that
shows a minimal before/after snippet for users migrating from `mkRemote` to
`lib.mkGitHooks`; mention the original call using `mkRemote` and the new
equivalent using `lib.mkGitHooks`, include any required option name changes
(e.g., hook list, formats) and a one-line instruction that they should replace
`mkRemote(...)` with `lib.mkGitHooks(...)` to make the upgrade path explicit for
existing users.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cc07103a-0fc0-4368-9cf2-86ca52ab0a73
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
.coderabbit.yaml.github/workflows/ci.yml.github/workflows/integration-test.ymlREADME.mdROADMAP.mdexamples/full.nixexamples/minimal.nixexamples/single-service.nixflake.nixlib/bootstrap.nixlib/default.nixlib/deploy.nixlib/hooks.nixlib/remote.nixlib/shell.nixlib/terraform.nixmodules/acme.nixmodules/base.nixmodules/digitalocean.nixmodules/disko.nixmodules/firewall.nixmodules/services.nixmodules/storage.nixtemplates/do-service/.github/workflows/ci.ymltemplates/do-service/.github/workflows/integration-test.ymltemplates/do-service/config/secrets.nixtemplates/do-service/flake.nixtemplates/do-service/infra/secrets.nixtemplates/do-service/infra/terraform.tfvars.exampletemplates/do-service/infra/variables.tftemplates/do-service/keys.nixtemplates/do-service/os.nixtemplates/do-service/services.nix
💤 Files with no reviewable changes (1)
- lib/remote.nix
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.nix
📄 CodeRabbit inference engine (AGENTS.md)
**/*.nix: Uselib.mkOptionwith proper types for all module options
Every module must be independently usable (no implicit dependencies on other omnix modules)
Options must live under theomnix.*namespace
Uselib.mkIf cfg.enablefor conditional configuration
Uselib.mkDefaultfor overridable defaults
Every configurable value must be a module option with a type and description
Preferlib.types.stroverlib.types.pathfor values that may come from terraform or other dynamic sources
Group related options under a singleomnix.<module>prefix
Functions take an attrset of required parameters
Return an attrset of derivations or values from library functions
Usepkgs.writeShellApplicationfor shell scripts (not rawwriteScript)
IncluderuntimeInputsin shell scripts and never assume tools are on PATH
All shell scripts must useset -eo pipefail
When changing module option interfaces, preserve the old option withlib.mkRenamedOptionModuleorlib.mkRemovedOptionModulewith a clear migration message
Files:
templates/do-service/infra/secrets.nixtemplates/do-service/services.nixmodules/storage.nixtemplates/do-service/config/secrets.nixmodules/digitalocean.nixtemplates/do-service/keys.nixmodules/disko.nixmodules/firewall.nixlib/default.nixmodules/base.nixtemplates/do-service/os.nixlib/hooks.nixexamples/single-service.nixexamples/minimal.nixexamples/full.nixmodules/acme.nixlib/bootstrap.nixlib/deploy.nixtemplates/do-service/flake.nixlib/terraform.nixmodules/services.nixlib/shell.nixflake.nix
*
⚙️ CodeRabbit configuration file
Focus on providing constructive criticism. Whenever you see a suboptimal approach, suggest more idiomatic or robust alternative(s). Flag potential footguns. Suggest FP alternatives to mutable/imperative code. Point out architectural flaws like leaky abstractions, tight coupling, wrong level of abstraction, poor type modeling, over-abstraction, unclear domain boundaries. Code should generally be organized based on business concerns rather than technical aspects - suggest improvements if you find violations. Point out gaps in test coverage but suggest tests that are not too coupled to the implementation and actually test domain invariants and business logic
Files:
README.mdROADMAP.mdflake.nix
**/*.md
⚙️ CodeRabbit configuration file
Focus on the contents of the docs and not on cosmetic things like markdown formatting. We use markdown files for various docs including but not limited to the north star system specification, SPEC.md, the plan for how to get there, ROADMAP.md, guidelines for AI contributors, AGENTS.md, project overview and instructions for human contributors, README.md. Think about the target audience of a document when deciding what comment to leave. For specifications and designs, suggest potential product, architectural, and UI/UX improvements. For plans, suggest changes that would make things more parallelizable and deliverable-focused. For instructions, suggest better rules and guidelines and point out missing instructions. In all cases, flag needless bloat, prefer clear concise writing, and consider the structure of the document and order of the sections
Files:
README.mdROADMAP.md
🧠 Learnings (17)
📓 Common learnings
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Include `runtimeInputs` in shell scripts and never assume tools are on PATH
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Use `pkgs.writeShellApplication` for shell scripts (not raw `writeScript`)
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Prefer `lib.types.str` over `lib.types.path` for values that may come from terraform or other dynamic sources
Applied to files:
templates/do-service/infra/secrets.nixmodules/storage.nixmodules/disko.nixtemplates/do-service/os.nixlib/terraform.nixlib/shell.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Functions take an attrset of required parameters
Applied to files:
templates/do-service/infra/secrets.nixmodules/disko.nixlib/default.nixmodules/base.nixlib/hooks.nixlib/bootstrap.nixlib/deploy.nixtemplates/do-service/flake.nixlib/terraform.nixmodules/services.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : When changing module option interfaces, preserve the old option with `lib.mkRenamedOptionModule` or `lib.mkRemovedOptionModule` with a clear migration message
Applied to files:
modules/storage.nixmodules/digitalocean.nixmodules/disko.nixlib/default.nixmodules/base.nixtemplates/do-service/os.nixmodules/acme.nixREADME.mdmodules/services.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Every configurable value must be a module option with a type and description
Applied to files:
modules/storage.nixmodules/disko.nixmodules/base.nixmodules/acme.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Use `lib.mkOption` with proper types for all module options
Applied to files:
modules/storage.nixmodules/digitalocean.nixmodules/disko.nixlib/default.nixmodules/base.nixtemplates/do-service/os.nixlib/hooks.nixmodules/acme.nixREADME.mdmodules/services.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Group related options under a single `omnix.<module>` prefix
Applied to files:
modules/storage.nixmodules/digitalocean.nixmodules/disko.nixmodules/base.nixtemplates/do-service/os.nixexamples/single-service.nixexamples/minimal.nixexamples/full.nixmodules/acme.nixREADME.mdtemplates/do-service/flake.nixROADMAP.mdmodules/services.nixflake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Use `lib.mkIf cfg.enable` for conditional configuration
Applied to files:
modules/storage.nixmodules/digitalocean.nixmodules/disko.nixlib/default.nixmodules/base.nixlib/hooks.nixmodules/services.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Every module must be independently usable (no implicit dependencies on other omnix modules)
Applied to files:
modules/digitalocean.nixmodules/disko.nixmodules/base.nixtemplates/do-service/os.nixexamples/minimal.nixmodules/acme.nixREADME.mdtemplates/do-service/flake.nixROADMAP.mdflake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Options must live under the `omnix.*` namespace
Applied to files:
modules/digitalocean.nixmodules/disko.nixmodules/base.nixtemplates/do-service/os.nixmodules/acme.nixREADME.mdtemplates/do-service/flake.nixflake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Template projects should pass `nix flake check`
Applied to files:
.github/workflows/ci.ymllib/default.nixmodules/base.nixtemplates/do-service/flake.nixROADMAP.mdflake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Use `lib.mkDefault` for overridable defaults
Applied to files:
lib/default.nixmodules/base.nixtemplates/do-service/os.nixlib/hooks.nixexamples/full.nixREADME.mdtemplates/do-service/flake.nixflake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Use `pkgs.writeShellApplication` for shell scripts (not raw `writeScript`)
Applied to files:
lib/default.nixmodules/base.nixlib/bootstrap.nixlib/deploy.nixtemplates/do-service/flake.nixlib/terraform.nixlib/shell.nixflake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Include `runtimeInputs` in shell scripts and never assume tools are on PATH
Applied to files:
lib/default.nixmodules/base.nixlib/hooks.nixlib/bootstrap.nixlib/deploy.nixtemplates/do-service/flake.nixlib/terraform.nixlib/shell.nixflake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Consumer repos using devenv dev shells require `nix flake check --impure` (devenv needs impure evaluation to resolve the working directory)
Applied to files:
lib/terraform.nixflake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : All shell scripts must use `set -eo pipefail`
Applied to files:
lib/terraform.nixlib/shell.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Return an attrset of derivations or values from library functions
Applied to files:
flake.nix
🔇 Additional comments (39)
.coderabbit.yaml (2)
3-13: Good explicit review behavior defaults.
profile: assertivewith explicitreview_details,sequence_diagrams,estimate_code_review_effort, andpoemtoggles is clear and schema-aligned.
19-27:auto_reviewconfiguration is valid and consistent.
base_branches: [".*"]andauto_pause_after_reviewed_commits: 7are correctly structured underreviews.auto_review.modules/storage.nix (1)
3-6: LGTM on the formatting refactor.The
let/inblock reformatting improves readability and aligns with standard Nix style conventions.modules/firewall.nix (1)
6-7: LGTM on the HTTP/HTTPS port management.The use of
lib.optionalto conditionally include ports is idiomatic Nix. The port composition inallowedTCPPortscorrectly maintains SSH (22) as always-allowed while making HTTP/HTTPS opt-in.Also applies to: 26-26
lib/deploy.nix (3)
3-10: LGTM on thetargetSystemparameterization.The new optional
targetSystem ? "x86_64-linux"parameter enables cross-architecture deployments while maintaining backward compatibility. Per the context snippets, existing callers rely on the default, which is appropriate since the template targets x86_64-linux droplets.
102-132: Shell scripts correctly usewriteShellApplicationwithruntimeInputs.All three wrapper scripts (
deployNixos,deployService,deployAll) follow the coding guidelines by usingpkgs.writeShellApplicationwith explicitruntimeInputs. ThewriteShellApplicationfunction automatically enablesset -euo pipefail, satisfying the shell safety requirement.Based on learnings: "Use
pkgs.writeShellApplicationfor shell scripts" and "IncluderuntimeInputsin shell scripts."
93-94: The--remote-buildflag correctly enables remote builds on non-Linux systems, which is appropriate for cross-compilation from macOS or other platforms. Be aware that this shifts the build workload to the target NixOS host; ensure adequate resources are available on the deployment target.templates/do-service/infra/variables.tf (2)
5-10: LGTM on thedo_tokenvalidation hardening.The
nullable = falseandtrimspace()validation correctly prevent empty or whitespace-only tokens, addressing the past review feedback.
35-39: LGTM on thevolume_size_gbinteger validation.The
floor()equality check ensures only whole-number GB values are accepted, correctly enforcing the integer domain constraint from past review feedback.modules/services.nix (2)
51-59: LGTM on theReadWritePathsfix.The
logDiris now correctly included inReadWritePathsalongsidedataDir, resolving the previous issue where dynamicUser services couldn't access their log directories.
116-120: LGTM on thelogDiroption.The new option follows conventions with proper type (
nullOr str), default (null), and description. It integrates correctly with tmpfiles rules and logrotate configuration.templates/do-service/services.nix (1)
4-4: LGTM on thedataDirpath update.Using
/mnt/data/my-serviceinstead of/mnt/data/prodprovides a more service-specific default that avoids potential collisions when multiple services are deployed.templates/do-service/infra/secrets.nix (1)
1-4: LGTM on the formatting refactor.The multi-line
let/inblock improves readability and follows standard Nix formatting conventions.templates/do-service/infra/terraform.tfvars.example (1)
1-2: LGTM on the security guidance comments.The instructional comments clearly direct users to use
nix run .#tfEditVarsfor encrypted variable management, preventing accidental plaintext credential storage.templates/do-service/.github/workflows/ci.yml (1)
54-57: Good hardening onHOST_KEYevaluation before writing known_hosts.The conditional guard on Lines 54-57 avoids appending invalid entries when key resolution fails.
lib/hooks.nix (1)
1-30: Looks good — clean hook composition and override model.The conditional
rustfmthook anddefaultHooks // extraHooksmerge are clear and predictable.examples/minimal.nix (1)
7-22: Good minimal example composition.Module set and explicit
omnix.base.stateVersionmake this example reproducible and clear.examples/single-service.nix (1)
17-37: LGTM for the single-service example.The configuration is coherent and uses explicit
omnix.*settings across included modules.examples/full.nix (1)
19-54: Strong end-to-end example configuration.Good coverage of module interactions (
base,storage,firewall,acme,services) with explicit values.lib/terraform.nix (3)
22-27: Addpkgs.nixtobuildInputsfornix evalcalls.The inherited
encryptStateandencryptVarshelpers fromlib/shell.nixinvokenix evalat runtime, butbuildInputs(used asruntimeInputsinmkTask) doesn't includepkgs.nix. Tasks using these helpers will fail when thenixbinary isn't on PATH.Suggested fix
buildInputs = [ pkgs.terraform pkgs.rage pkgs.jq + pkgs.nix ] ++ (if ragenixPkg != null then [ ragenixPkg ] else [ ]);Based on learnings: "Include
runtimeInputsin shell scripts and never assume tools are on PATH".
64-70: LGTM!The
mkTaskhelper correctly useswriteShellApplicationwithruntimeInputs = buildInputs, following the coding guidelines for shell script generation.
132-143: LGTM!The
remotescript correctly declares its ownruntimeInputswith the specific tools it needs (rage,jq,openssh), separate from the Terraform tasks.templates/do-service/.github/workflows/integration-test.yml (2)
92-94: Don't suppress destroy failures.
if: always()already ensures teardown runs regardless of prior step outcomes. The trailing|| truemasks cleanup failures, potentially leaving droplets alive while the workflow reports success.Suggested fix
- name: Teardown if: always() - run: nix run .#tfDestroy -- -i ~/.ssh/id_ed25519 || true + run: nix run .#tfDestroy -- -i ~/.ssh/id_ed25519
28-40: LGTM!The workflow correctly:
- Wires
inputs.droplet_sizeandinputs.regiontoTF_VAR_*environment variables- Pins DeterminateSystems actions to stable release tags (
@v17,@v8)lib/shell.nix (4)
7-13: Add validation for-iargument.
parseIdentityshifts 2 positions after checking only$1, but doesn't verify$2exists. If called with-ibut no second argument,$identitybecomes empty and subsequent commands fail confusingly.Proposed fix
parseIdentity = '' identity=~/.ssh/id_ed25519 if [ "''${1:-}" = "-i" ]; then + if [ -z "''${2:-}" ]; then + echo "Error: -i requires an identity file argument" >&2 + exit 1 + fi identity="$2" shift 2 fi '';
22-27: Validate recipients before encrypting.If
nix evalfails orroles.infrais empty,rageencrypts to zero recipients, producing a file that cannot be decrypted. The pipeline silently continues, masking the error.Proposed fix
encryptState = '' if [ -f ${tfState} ]; then - nix eval --raw --file ${keysFile} roles.infra --apply 'builtins.concatStringsSep "\n"' \ - | rage -e -R /dev/stdin -o ${tfState}.age ${tfState} + recipients=$(nix eval --raw --file ${keysFile} roles.infra --apply 'builtins.concatStringsSep "\n"') + if [ -z "$recipients" ]; then + echo "Error: No recipients found in ${keysFile} roles.infra" >&2 + exit 1 + fi + echo "$recipients" | rage -e -R /dev/stdin -o ${tfState}.age ${tfState} fi '';
34-37: Add file existence check and recipient validation.
encryptVarshas the same silent-failure risk asencryptState, and additionally doesn't verify the source file exists before attempting encryption.Proposed fix
encryptVars = '' + if [ ! -f ${tfVars} ]; then + echo "Error: ${tfVars} not found" >&2 + exit 1 + fi - nix eval --raw --file ${keysFile} roles.infra --apply 'builtins.concatStringsSep "\n"' \ - | rage -e -R /dev/stdin -o ${tfVars}.age ${tfVars} + recipients=$(nix eval --raw --file ${keysFile} roles.infra --apply 'builtins.concatStringsSep "\n"') + if [ -z "$recipients" ]; then + echo "Error: No recipients found in ${keysFile} roles.infra" >&2 + exit 1 + fi + echo "$recipients" | rage -e -R /dev/stdin -o ${tfVars}.age ${tfVars} '';
39-45: LGTM!
resolveIpcorrectly usesjq -e -rto fail fast on missing/null values, and includes a cleanup trap for the decrypted state file..github/workflows/integration-test.yml (2)
133-136: Don't suppress destroy failures.
if: always()guarantees the teardown step runs. The trailing|| truecan mask Terraform destroy failures, leaving infrastructure orphaned while the workflow reports success.Suggested fix
- name: Teardown if: always() working-directory: ${{ env.TEST_DIR }} - run: nix run .#tfDestroy -- -i ~/.ssh/id_ed25519 || true + run: nix run .#tfDestroy -- -i ~/.ssh/id_ed25519
31-43: LGTM!The workflow correctly wires
workflow_dispatchinputs to Terraform viaTF_VAR_*environment variables, and pins DeterminateSystems actions to stable release tags (@v17,@v8).ROADMAP.md (3)
111-113: ClarifymkIntegrationTestdeliverable.The description mentions parameterization by "project-specific config" but doesn't specify the expected signature or how consumers would invoke it. Adding a brief usage example would clarify scope.
3-21: LGTM!PR review findings are now properly collapsed in a
<details>section, keeping the roadmap focused on forward-looking work while preserving the historical record.
52-64: LGTM!The Mermaid diagram now correctly shows parallel paths: B feeds both C/D (for CLI) and F/G (NixOS module/deploy-rs activation) independently, reflecting that these can be developed in parallel.
flake.nix (4)
109-116: Consider deriving dev-shell toolchain from the hook check.The
shellHookcomes fromchecks.git-hooks, but packages are manually maintained. This can drift from hook configuration. The git-hooks.nix library providesenabledPackagesthat automatically includes the tools configured for hooks.Suggested refactor
- devShells.default = pkgs.mkShell { - inherit (self.checks.${system}.git-hooks) shellHook; - packages = [ - pkgs.nixfmt - pkgs.deadnix - pkgs.taplo - ]; - }; + devShells.default = + let + inherit (self.checks.${system}.git-hooks) shellHook enabledPackages; + in + pkgs.mkShell { + inherit shellHook; + buildInputs = enabledPackages; + };
56-69: LGTM!Good additions:
- New
acmemodule for ACME/Let's Encrypt supportnixosModules.defaultnow bundles upstreamdisko.nixosModules.diskoandragenix.nixosModules.default, reducing boilerplate for consumers
98-99: LGTM!Replacing the silent
nullfallback with an explicitthrowprovides a clear error message when disko isn't available for the target system, improving debuggability.
102-105: Thegit-hooks.lib.${system}.runAPI call is correctly formatted. ThemkGitHooksfunction returns an attribute set with hook names as keys and their configurations as values, which matches the expected signature for git-hooks.nix'srunfunction. Theinherit hookssyntax correctly passes this attribute set to the expectedhooksparameter.templates/do-service/flake.nix (2)
95-105: LGTM!The
resolveIppackage correctly useswriteShellApplicationwith appropriateruntimeInputs(rage for decryption, jq for JSON extraction). The shell snippet frominfraPkgs.resolveIpsets$host_ipwhich is then echoed.
27-30: LGTM!Simplified module imports to just
omnix.nixosModules.default(which now includes upstream disko and ragenix) plus./os.nix. This aligns with the refactorednixosModules.defaultin the main flake.
| path_instructions: | ||
| - path: "*" | ||
| instructions: >- |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In CodeRabbit reviews.path_instructions.pathglob matching, does"*" match only top-level files or all nested files recursively? What is the recommended pattern to target all files in a repository?
💡 Result:
In CodeRabbit reviews.path_instructions.path glob matching, which uses minimatch, the pattern "" matches only top-level files (and directories) in the current directory, not nested files recursively. It matches zero or more characters excluding path separators (/), so it does not cross directory boundaries. The recommended pattern to target all files in a repository recursively is "/" or "". Examples from docs: - "src/controllers/" targets all files under src/controllers recursively. - "tests/" targets all under tests recursively. - "docs//.md" targets all .md files recursively under docs. - "/.ts" for all TypeScript files repo-wide.
Citations:
- 1: https://docs.coderabbit.ai/configuration/path-instructions
- 2: https://coderabbit.mintlify.app/configuration/path-instructions
- 3: https://isaacs.github.io/minimatch/
- 4: https://aicodereview.cc/blog/coderabbit-configuration
- 5: https://cdn.jsdelivr.net/npm/github-pr-automation@0.5.2/.coderabbit.yaml
Use the recommended glob pattern for repository-wide path instructions.
Line 33 uses path: "*", which matches only top-level files, not nested files recursively. For CodeRabbit path instructions that should apply repository-wide, use "/*" or an empty string "" instead (minimatch patterns).
Suggested change
- - path: "*"
+ - path: "/*"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.coderabbit.yaml around lines 32 - 34, The path_instructions entry currently
uses path: "*" which only matches top-level files; change the repository-wide
glob in the .coderabbit.yaml path_instructions block (the path_instructions key
and its path: "*" entry) to use a recursive pattern such as path: "/*" or an
empty string "" so the instructions apply to all nested files; update that path
value accordingly and keep the rest of the path_instructions configuration
unchanged.
| - uses: DeterminateSystems/nix-installer-action@v17 | ||
| - uses: DeterminateSystems/magic-nix-cache-action@v8 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Resolve immutable SHAs for the currently used action tags.
for spec in \
"DeterminateSystems/nix-installer-action v17" \
"DeterminateSystems/magic-nix-cache-action v8"
do
repo=$(echo "$spec" | awk '{print $1}')
tag=$(echo "$spec" | awk '{print $2}')
sha=$(gh api "repos/${repo}/git/ref/tags/${tag}" --jq '.object.sha')
echo "${repo}@${tag} -> ${sha}"
doneRepository: data-cartel/omnix
Length of output: 91
🏁 Script executed:
cat -n .github/workflows/ci.ymlRepository: data-cartel/omnix
Length of output: 1369
Pin all third-party actions to commit SHAs, not version tags.
Lines 16, 17, 18, 31, 32, 33 use mutable version tags (@v4, @v17, @v8). For stronger supply-chain integrity and reproducibility, pin each action to its full commit SHA (optionally annotate with the tag in a comment). This applies to:
actions/checkout@v4(lines 16, 31)DeterminateSystems/nix-installer-action@v17(lines 17, 32)DeterminateSystems/magic-nix-cache-action@v8(lines 18, 33)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 17 - 18, The workflow pins third-party
GitHub Actions using mutable tags (actions/checkout@v4,
DeterminateSystems/nix-installer-action@v17,
DeterminateSystems/magic-nix-cache-action@v8); replace each tag with the
corresponding full commit SHA for every usage of those actions in the file (both
occurrences), optionally keeping the tag as a trailing comment for readability,
so each uses the immutable ref (full SHA) instead of `@vX` to improve supply-chain
integrity and reproducibility.
| - name: Setup SSH identity | ||
| run: | | ||
| mkdir -p ~/.ssh | ||
| chmod 700 ~/.ssh | ||
| echo "${{ secrets.SSH_KEY }}" > ~/.ssh/id_ed25519 | ||
| chmod 600 ~/.ssh/id_ed25519 |
There was a problem hiding this comment.
Secret name differs from template workflow.
This workflow uses secrets.SSH_KEY while templates/do-service/.github/workflows/integration-test.yml uses secrets.SSH_PRIVATE_KEY. This inconsistency could cause confusion when setting up repository secrets.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/integration-test.yml around lines 45 - 50, The workflow
step "Setup SSH identity" uses the wrong secret name; change the environment
variable reference in the echo line from ${{ secrets.SSH_KEY }} to ${{
secrets.SSH_PRIVATE_KEY }} (and update any other occurrences of secrets.SSH_KEY
in this workflow) so it matches the template's secret naming; ensure the step
that writes ~/.ssh/id_ed25519 still uses the updated secret reference and keep
existing chmod/mkdir lines unchanged.
| security.acme = { | ||
| acceptTerms = true; | ||
| defaults.email = cfg.email; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Make ACME terms acceptance overridable.
Line 26 hard-sets security.acme.acceptTerms = true;, which prevents downstream override priority. Use lib.mkDefault here.
♻️ Proposed fix
security.acme = {
- acceptTerms = true;
+ acceptTerms = lib.mkDefault true;
defaults.email = cfg.email;
};As per coding guidelines, Use lib.mkDefault for overridable defaults.
📝 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.
| security.acme = { | |
| acceptTerms = true; | |
| defaults.email = cfg.email; | |
| security.acme = { | |
| acceptTerms = lib.mkDefault true; | |
| defaults.email = cfg.email; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/acme.nix` around lines 25 - 27, security.acme.acceptTerms is hard-set
to true which prevents downstream overrides; change the assignment to use
lib.mkDefault so the default value can be overridden (e.g., replace the direct
true with lib.mkDefault true for security.acme.acceptTerms while keeping
defaults.email = cfg.email unchanged), referencing the security.acme.acceptTerms
attribute and lib.mkDefault helper.
| enableHTTP = lib.mkEnableOption "port 80 (HTTP, needed for ACME challenges)"; | ||
| enableHTTPS = lib.mkEnableOption "port 443 (HTTPS)"; |
There was a problem hiding this comment.
Consider adding an assertion in modules/acme.nix to enforce enableHTTP.
The enableHTTP option correctly opens port 80 for ACME HTTP-01 challenges. However, per modules/acme.nix (context snippet 1), a user can enable ACME while leaving enableHTTP = false, silently breaking certificate validation.
Consider adding an assertion in acme.nix:
assertions = [
# ... existing assertions
{
assertion = !cfg.enable || config.omnix.firewall.enableHTTP;
message = "omnix.acme requires omnix.firewall.enableHTTP = true for HTTP-01 challenges";
}
];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/firewall.nix` around lines 13 - 14, Add an assertion in
modules/acme.nix to require omnix.firewall.enableHTTP when ACME is enabled:
inside the existing assertions attribute, add an assertion that checks
cfg.enable implies config.omnix.firewall.enableHTTP and provide a clear message
like "omnix.acme requires omnix.firewall.enableHTTP = true for HTTP-01
challenges"; reference the assertions array and the
cfg.enable/config.omnix.firewall.enableHTTP symbols so the module will fail fast
if ACME is turned on but firewall HTTP is disabled.
| - uses: DeterminateSystems/nix-installer-action@v17 | ||
| - uses: DeterminateSystems/magic-nix-cache-action@v8 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Resolve SHAs for tags used in the template CI workflow.
for spec in \
"DeterminateSystems/nix-installer-action v17" \
"DeterminateSystems/magic-nix-cache-action v8"
do
repo=$(echo "$spec" | awk '{print $1}')
tag=$(echo "$spec" | awk '{print $2}')
sha=$(gh api "repos/${repo}/git/ref/tags/${tag}" --jq '.object.sha')
echo "${repo}@${tag} -> ${sha}"
doneRepository: data-cartel/omnix
Length of output: 91
🏁 Script executed:
find . -path "*templates/do-service/.github/workflows/ci.yml" -type fRepository: data-cartel/omnix
Length of output: 108
🏁 Script executed:
cat -n ./templates/do-service/.github/workflows/ci.ymlRepository: data-cartel/omnix
Length of output: 2112
Use immutable SHAs for all third-party GitHub Actions.
This workflow pins GitHub Actions to mutable release tags across all three jobs. Lines 16, 17-18, 25, 26-27, 42, 43-44 use @v tags (actions/checkout@v4, DeterminateSystems/nix-installer-action@v17, DeterminateSystems/magic-nix-cache-action@v8), which can be moved or force-pushed by maintainers. Pin each action to its immutable commit SHA to prevent silent supply-chain drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/do-service/.github/workflows/ci.yml` around lines 17 - 18, Replace
mutable release tags with immutable commit SHAs for all third-party GitHub
Actions referenced (e.g., actions/checkout@v4,
DeterminateSystems/nix-installer-action@v17,
DeterminateSystems/magic-nix-cache-action@v8 and any other `@v` tags in the
workflow) by looking up the corresponding commit SHA on each action's GitHub
repo and updating the workflow references to use @<commit-sha>; ensure every
occurrence of the mutable tag is updated so no action remains pinned to a
mutable release.
| - name: Wait for SSH | ||
| run: | | ||
| retries=0 | ||
| until ssh -o StrictHostKeyChecking=no -o ConnectTimeout=5 \ | ||
| -i ~/.ssh/id_ed25519 "root@$HOST_IP" true 2>/dev/null; do | ||
| retries=$((retries + 1)) | ||
| if [ "$retries" -ge 30 ]; then | ||
| echo "SSH not available after 2.5 minutes" >&2 | ||
| exit 1 | ||
| fi | ||
| sleep 5 | ||
| done |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding BatchMode=yes to SSH options.
Adding BatchMode=yes prevents SSH from hanging on unexpected interactive prompts, making the retry loop more deterministic in CI.
Suggested fix
- name: Wait for SSH
run: |
retries=0
- until ssh -o StrictHostKeyChecking=no -o ConnectTimeout=5 \
+ until ssh -o StrictHostKeyChecking=no -o ConnectTimeout=5 -o BatchMode=yes \
-i ~/.ssh/id_ed25519 "root@$HOST_IP" true 2>/dev/null; do📝 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.
| - name: Wait for SSH | |
| run: | | |
| retries=0 | |
| until ssh -o StrictHostKeyChecking=no -o ConnectTimeout=5 \ | |
| -i ~/.ssh/id_ed25519 "root@$HOST_IP" true 2>/dev/null; do | |
| retries=$((retries + 1)) | |
| if [ "$retries" -ge 30 ]; then | |
| echo "SSH not available after 2.5 minutes" >&2 | |
| exit 1 | |
| fi | |
| sleep 5 | |
| done | |
| - name: Wait for SSH | |
| run: | | |
| retries=0 | |
| until ssh -o StrictHostKeyChecking=no -o ConnectTimeout=5 -o BatchMode=yes \ | |
| -i ~/.ssh/id_ed25519 "root@$HOST_IP" true 2>/dev/null; do | |
| retries=$((retries + 1)) | |
| if [ "$retries" -ge 30 ]; then | |
| echo "SSH not available after 2.5 minutes" >&2 | |
| exit 1 | |
| fi | |
| sleep 5 | |
| done |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/do-service/.github/workflows/integration-test.yml` around lines 61
- 72, In the "Wait for SSH" workflow step the ssh command can hang on
interactive prompts; update the ssh invocation (the until ssh ... true loop) to
include the option -o BatchMode=yes alongside -o StrictHostKeyChecking=no and -o
ConnectTimeout=5 so SSH exits immediately on auth prompts and the retry loop
remains deterministic in CI.
| omnix.digitalocean.enable = true; | ||
| omnix.base = { | ||
| enable = true; | ||
| stateVersion = "24.11"; |
There was a problem hiding this comment.
Align template stateVersion with the pinned NixOS release.
Line 11 sets "24.11" while this repository pin/examples are on 25.11, which creates avoidable drift for new deployments.
🔧 Proposed fix
- stateVersion = "24.11";
+ stateVersion = "25.11";📝 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.
| stateVersion = "24.11"; | |
| stateVersion = "25.11"; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@templates/do-service/os.nix` at line 11, Update the stateVersion value to
match the repository's pinned NixOS release: change the stateVersion declaration
(symbol: stateVersion) from "24.11" to "25.11" so the template aligns with the
pinned/examples release and avoids deployment drift.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
flake.nix (1)
124-130: 🧹 Nitpick | 🔵 TrivialDerive the dev shell from the hook check.
This still hardcodes the tool list while only reusing
shellHook. Thegit-hooks.nixflake example inherits bothshellHookandenabledPackagesfrom the check, which keeps the shell aligned with the configured hooks and avoids mirroring that list here by hand. (github.com)♻️ Suggested refactor
- devShells.default = pkgs.mkShell { - inherit (self.checks.${system}.git-hooks) shellHook; - packages = [ - pkgs.nixfmt - pkgs.deadnix - pkgs.taplo - ]; - }; + devShells.default = + let + inherit (self.checks.${system}.git-hooks) shellHook enabledPackages; + in + pkgs.mkShell { + inherit shellHook; + buildInputs = enabledPackages; + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flake.nix` around lines 124 - 130, The dev shell currently hardcodes packages while only inheriting shellHook from the git-hooks check; change it to derive both shellHook and enabledPackages from the check so the shell stays in sync with the hook config: in the devShells.default block inherit (self.checks.${system}.git-hooks) shellHook enabledPackages and set the shell's packages to the inherited enabledPackages (rather than listing pkgs.nixfmt, pkgs.deadnix, pkgs.taplo) so updates to git-hooks enabledPackages automatically propagate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flake.nix`:
- Around line 113-115: The current packages.disko uses a hard throw
(disko.packages.${system}.default or (throw ...)) which causes flake evaluation
failures on unsupported systems; replace the throw with a conditional/optional
inclusion so unsupported systems simply omit the attribute. Change the
assignment so you first check for the presence of disko.packages for the current
system (e.g., via lib.hasAttr or by using the fallback
(disko.packages.${system}.default or null)) and then use lib.optionalAttrs or an
if expression to add packages.disko only when that value is non-null; update the
expression that currently references disko.packages.${system}.default and the
surrounding packages map to use this conditional logic (symbols: packages.disko,
disko.packages.${system}.default, flake-utils.lib.eachDefaultSystem).
---
Duplicate comments:
In `@flake.nix`:
- Around line 124-130: The dev shell currently hardcodes packages while only
inheriting shellHook from the git-hooks check; change it to derive both
shellHook and enabledPackages from the check so the shell stays in sync with the
hook config: in the devShells.default block inherit
(self.checks.${system}.git-hooks) shellHook enabledPackages and set the shell's
packages to the inherited enabledPackages (rather than listing pkgs.nixfmt,
pkgs.deadnix, pkgs.taplo) so updates to git-hooks enabledPackages automatically
propagate.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0ba23601-453f-486d-ab3e-014dc7bd2d8f
📒 Files selected for processing (1)
flake.nix
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.nix
📄 CodeRabbit inference engine (AGENTS.md)
**/*.nix: Uselib.mkOptionwith proper types for all module options
Every module must be independently usable (no implicit dependencies on other omnix modules)
Options must live under theomnix.*namespace
Uselib.mkIf cfg.enablefor conditional configuration
Uselib.mkDefaultfor overridable defaults
Every configurable value must be a module option with a type and description
Preferlib.types.stroverlib.types.pathfor values that may come from terraform or other dynamic sources
Group related options under a singleomnix.<module>prefix
Functions take an attrset of required parameters
Return an attrset of derivations or values from library functions
Usepkgs.writeShellApplicationfor shell scripts (not rawwriteScript)
IncluderuntimeInputsin shell scripts and never assume tools are on PATH
All shell scripts must useset -eo pipefail
When changing module option interfaces, preserve the old option withlib.mkRenamedOptionModuleorlib.mkRemovedOptionModulewith a clear migration message
Files:
flake.nix
*
⚙️ CodeRabbit configuration file
Focus on providing constructive criticism. Whenever you see a suboptimal approach, suggest more idiomatic or robust alternative(s). Flag potential footguns. Suggest FP alternatives to mutable/imperative code. Point out architectural flaws like leaky abstractions, tight coupling, wrong level of abstraction, poor type modeling, over-abstraction, unclear domain boundaries. Code should generally be organized based on business concerns rather than technical aspects - suggest improvements if you find violations. Point out gaps in test coverage but suggest tests that are not too coupled to the implementation and actually test domain invariants and business logic
Files:
flake.nix
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Use `pkgs.writeShellApplication` for shell scripts (not raw `writeScript`)
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Include `runtimeInputs` in shell scripts and never assume tools are on PATH
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Template projects should pass `nix flake check`
Applied to files:
flake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Consumer repos using devenv dev shells require `nix flake check --impure` (devenv needs impure evaluation to resolve the working directory)
Applied to files:
flake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Include `runtimeInputs` in shell scripts and never assume tools are on PATH
Applied to files:
flake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Use `pkgs.writeShellApplication` for shell scripts (not raw `writeScript`)
Applied to files:
flake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Every module must be independently usable (no implicit dependencies on other omnix modules)
Applied to files:
flake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Use `lib.mkDefault` for overridable defaults
Applied to files:
flake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Return an attrset of derivations or values from library functions
Applied to files:
flake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Group related options under a single `omnix.<module>` prefix
Applied to files:
flake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Options must live under the `omnix.*` namespace
Applied to files:
flake.nix
🔇 Additional comments (2)
flake.nix (2)
37-62: Helpful example surface.Promoting the examples to first-class
nixosConfigurationsmakes the flake easier to discover and consume from the outside.
71-85: Nice self-contained default module.Bundling upstream
disko/ragenixwith the local modules makesnixosModules.defaultusable as a single import instead of relying on consumers to remember extra upstream modules. As per coding guidelines, "Every module must be independently usable (no implicit dependencies on other omnix modules)".
| packages.disko = | ||
| disko.packages.${system}.default or (throw "disko package not available for ${system}"); | ||
| packages.ragenix = ragenix.packages.${system}.default; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rev=$(python - <<'PY'
import json
with open("flake.lock") as f:
lock = json.load(f)
print(lock["nodes"]["disko"]["locked"]["rev"])
PY
)
for file in flake.nix package.nix default.nix; do
echo "== ${file} @ ${rev} =="
curl -Lsf "https://raw.githubusercontent.com/nix-community/disko/${rev}/${file}" \
| rg -n 'supportedSystems|each(Default)?System|x86_64-darwin|aarch64-darwin|linux|packages|default' || true
echo
doneRepository: data-cartel/omnix
Length of output: 1295
🏁 Script executed:
sed -n '100,125p' flake.nixRepository: data-cartel/omnix
Length of output: 883
🏁 Script executed:
rg -A 5 'hasAttrByPath|optionalAttrs' /dev/stdin <<'EOF'
lib.optionalAttrs
pkgs.lib.optionalAttrs
hasAttrByPath
EOFRepository: data-cartel/omnix
Length of output: 111
🏁 Script executed:
# Check flake-utils.lib.eachDefaultSystem default systems
git ls-files | grep -i flake-utils | head -5 || echo "Checking web source for flake-utils defaults"Repository: data-cartel/omnix
Length of output: 43
🌐 Web query:
flake-utils lib.eachDefaultSystem default supported systems
💡 Result:
The default supported systems for flake-utils.lib.eachDefaultSystem are ["x86_64-linux", "aarch64-linux", "x86_64-darwin", "aarch64-darwin"]. This list is defined in flake-utils as defaultSystems and can be overridden via the "systems" input.
Citations:
- 1: https://nixcademy.com/posts/1000-instances-of-flake-utils/
- 2: https://mynixos.com/flake-utils
- 3: https://github.com/numtide/flake-utils
- 4: https://github.com/numtide/flake-utils/blob/main/README.md
Don't throw from a per-system package slot.
flake-utils.lib.eachDefaultSystem includes x86_64-darwin, but the pinned disko revision only exports packages for x86_64-linux, i686-linux, aarch64-linux, riscv64-linux, and aarch64-darwin. Because nix flake check evaluates packages.<system>.<name> for all systems, this hard throw turns packages.disko into a flake-evaluation failure on Intel macOS instead of just omitting the attribute.
Replace the throw with conditional inclusion:
Suggested fix
- packages.disko =
- disko.packages.${system}.default or (throw "disko package not available for ${system}");
- packages.ragenix = ragenix.packages.${system}.default;
+ packages =
+ {
+ ragenix = ragenix.packages.${system}.default;
+ }
+ // lib.optionalAttrs (disko.packages ? ${system}) {
+ disko = disko.packages.${system}.default;
+ };📝 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.
| packages.disko = | |
| disko.packages.${system}.default or (throw "disko package not available for ${system}"); | |
| packages.ragenix = ragenix.packages.${system}.default; | |
| packages = | |
| { | |
| ragenix = ragenix.packages.${system}.default; | |
| } | |
| // lib.optionalAttrs (disko.packages ? ${system}) { | |
| disko = disko.packages.${system}.default; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flake.nix` around lines 113 - 115, The current packages.disko uses a hard
throw (disko.packages.${system}.default or (throw ...)) which causes flake
evaluation failures on unsupported systems; replace the throw with a
conditional/optional inclusion so unsupported systems simply omit the attribute.
Change the assignment so you first check for the presence of disko.packages for
the current system (e.g., via lib.hasAttr or by using the fallback
(disko.packages.${system}.default or null)) and then use lib.optionalAttrs or an
if expression to add packages.disko only when that value is non-null; update the
expression that currently references disko.packages.${system}.default and the
surrounding packages map to use this conditional logic (symbols: packages.disko,
disko.packages.${system}.default, flake-utils.lib.eachDefaultSystem).
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flake.nix`:
- Around line 107-110: The flake currently calls lib.mkGitHooks with an empty
attribute set which uses defaults; update the documentation/comments near the
mkGitHooks invocation to note that mkGitHooks supports customization via
extraHooks and rustToolchain (see lib/hooks.nix) so downstream consumers know
they can pass those keys (e.g., extraHooks = ... or rustToolchain = ...) to
customize hooks and Rust toolchain behavior; include a short example in the
comment showing the attribute names and purpose.
- Around line 72-86: The convenience bundle `default` currently imports upstream
modules (e.g., disko.nixosModules.disko and ragenix.nixosModules.default) which
can surprise users who also import those upstream modules directly; add a short
note to the repository documentation (README or SPEC.md) stating that
nixosModules.default includes upstream dependencies and that duplicate imports
are handled by the NixOS module system, and include an example or sentence
advising users to avoid redundant explicit imports when using the convenience
`default` bundle (refer to the `default` attribute and the symbols
disko.nixosModules.disko, ragenix.nixosModules.default, and
nixosModules.default).
- Around line 88-95: The lib import is passing an unused parameter `git-hooks`
into the `lib` function; remove `git-hooks` from the inherit clause in the `lib
= import ./lib { inherit ... }` block (leave `deploy-rs` and `nixos-anywhere`
intact) and verify there are no other references to `git-hooks` in the flake so
the call matches `lib/default.nix`'s actual parameters.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 56e9420e-bd2e-4316-b822-4ca03a2409b2
📒 Files selected for processing (3)
.coderabbit.yamlexamples/minimal.nixflake.nix
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.nix
📄 CodeRabbit inference engine (AGENTS.md)
**/*.nix: Uselib.mkOptionwith proper types for all module options
Every module must be independently usable (no implicit dependencies on other omnix modules)
Options must live under theomnix.*namespace
Uselib.mkIf cfg.enablefor conditional configuration
Uselib.mkDefaultfor overridable defaults
Every configurable value must be a module option with a type and description
Preferlib.types.stroverlib.types.pathfor values that may come from terraform or other dynamic sources
Group related options under a singleomnix.<module>prefix
Functions take an attrset of required parameters
Return an attrset of derivations or values from library functions
Usepkgs.writeShellApplicationfor shell scripts (not rawwriteScript)
IncluderuntimeInputsin shell scripts and never assume tools are on PATH
All shell scripts must useset -eo pipefail
When changing module option interfaces, preserve the old option withlib.mkRenamedOptionModuleorlib.mkRemovedOptionModulewith a clear migration message
Files:
examples/minimal.nixflake.nix
*
⚙️ CodeRabbit configuration file
Focus on providing constructive criticism. Whenever you see a suboptimal approach, suggest more idiomatic or robust alternative(s). Flag potential footguns. Suggest FP alternatives to mutable/imperative code. Point out architectural flaws like leaky abstractions, tight coupling, wrong level of abstraction, poor type modeling, over-abstraction, unclear domain boundaries. Code should generally be organized based on business concerns rather than technical aspects - suggest improvements if you find violations. Point out gaps in test coverage but suggest tests that are not too coupled to the implementation and actually test domain invariants and business logic
Files:
flake.nix
🧠 Learnings (10)
📓 Common learnings
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Include `runtimeInputs` in shell scripts and never assume tools are on PATH
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Use `pkgs.writeShellApplication` for shell scripts (not raw `writeScript`)
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Every module must be independently usable (no implicit dependencies on other omnix modules)
Applied to files:
examples/minimal.nixflake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Group related options under a single `omnix.<module>` prefix
Applied to files:
examples/minimal.nixflake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Template projects should pass `nix flake check`
Applied to files:
flake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Consumer repos using devenv dev shells require `nix flake check --impure` (devenv needs impure evaluation to resolve the working directory)
Applied to files:
flake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Include `runtimeInputs` in shell scripts and never assume tools are on PATH
Applied to files:
flake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Use `pkgs.writeShellApplication` for shell scripts (not raw `writeScript`)
Applied to files:
flake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Use `lib.mkDefault` for overridable defaults
Applied to files:
flake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Return an attrset of derivations or values from library functions
Applied to files:
flake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Options must live under the `omnix.*` namespace
Applied to files:
flake.nix
🔇 Additional comments (12)
.coderabbit.yaml (5)
32-34: Use**/*for repository-wide path instructions.The
path: "*"pattern only matches top-level files, not files in subdirectories. For instructions that should apply repository-wide, use**/*instead.
3-9: Documentation comments improve maintainability.The inline comments explaining configuration options (
profile,review_details) are helpful for future maintainers. LGTM.
14-17: Clear explanation of the request changes workflow.The comment accurately describes the behavior of
request_changes_workflow. LGTM.
19-27: Auto-review configuration is valid and well-documented.
base_branches: [".*"]correctly enables reviews on all branches using regex.auto_pause_after_reviewed_commits: 7is a reasonable threshold to reduce noise on active PRs.- Comments accurately describe the settings.
44-55: Markdown-specific instructions are well-structured.The
**/*.mdpattern correctly uses recursive matching, and the instructions provide clear guidance tailored to different document types (specs, roadmaps, agent guidelines, READMEs). The focus on content over formatting and audience-aware commenting is appropriate.flake.nix (6)
112-114: Hardthrowbreaks flake evaluation on unsupported systems.This was flagged in a previous review.
flake-utils.lib.eachDefaultSystemincludesx86_64-darwin, but disko may not export packages for all systems. Thethrowcausesnix flake checkto fail instead of gracefully omitting the attribute.
123-130: Dev shell packages are manually maintained and may drift from hooks configuration.This was flagged in a previous review. The packages list (
nixfmt,deadnix,taplo) is manually maintained and will diverge fromlib/hooks.nixif hooks are added/removed. Consider deriving packages from the git-hooks check'senabledPackages.
19-21: Git-hooks input properly configured.The input correctly follows
nixpkgsto avoid version conflicts. This is the standard pattern forgit-hooks.nixintegration.
37-61: Clean nixosConfigurations structure.Each example receives only the flake inputs it requires, following the principle of minimal dependencies. This makes the dependency graph clear and examples easier to reason about.
116-119: Correct git-hooks integration.The
git-hooks.lib.${system}.runpattern withsrc = selffollows the standardgit-hooks.nixusage. The hooks configuration fromself.lib.mkGitHooks { }correctly produces the attribute set expected by the library.
121-121: Formatter correctly configured.Using
pkgs.nixfmtas the formatter aligns with thenixfmthook inlib/hooks.nix, ensuring consistent formatting between CI and local development.examples/minimal.nix (1)
1-21: Well-structured minimal example.This example correctly demonstrates:
- Independent module imports (both upstream
disko.nixosModules.diskoandself.nixosModules.disko)- Proper use of
omnix.*namespace for configuration- Required
stateVersionis explicitly set (matching the nixos-25.11 input)- Uses default disk device (
/dev/vda) which is appropriate for a minimal example
| # Convenience: all modules at once (includes upstream disko + ragenix) | ||
| default = { | ||
| imports = [ | ||
| disko.nixosModules.disko | ||
| ragenix.nixosModules.default | ||
| self.nixosModules.disko | ||
| self.nixosModules.digitalocean | ||
| self.nixosModules.base | ||
| self.nixosModules.storage | ||
| self.nixosModules.services | ||
| self.nixosModules.firewall | ||
| self.nixosModules.acme | ||
| ]; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Bundling upstream modules in default is convenient but document the behavior.
Including disko.nixosModules.disko and ragenix.nixosModules.default in the convenience bundle is helpful, but users who also import these upstream modules directly in their configuration may be surprised. The NixOS module system handles duplicate imports gracefully, but this should be documented.
Consider adding a brief note in README or SPEC.md that nixosModules.default includes upstream dependencies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flake.nix` around lines 72 - 86, The convenience bundle `default` currently
imports upstream modules (e.g., disko.nixosModules.disko and
ragenix.nixosModules.default) which can surprise users who also import those
upstream modules directly; add a short note to the repository documentation
(README or SPEC.md) stating that nixosModules.default includes upstream
dependencies and that duplicate imports are handled by the NixOS module system,
and include an example or sentence advising users to avoid redundant explicit
imports when using the convenience `default` bundle (refer to the `default`
attribute and the symbols disko.nixosModules.disko,
ragenix.nixosModules.default, and nixosModules.default).
| let | ||
| pkgs = import nixpkgs { inherit system; }; | ||
| hooks = self.lib.mkGitHooks { }; | ||
| in |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider documenting hook customization.
mkGitHooks accepts extraHooks and rustToolchain parameters per lib/hooks.nix. The current usage with { } uses defaults only. This is correct, but downstream consumers may not know these customization options exist.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flake.nix` around lines 107 - 110, The flake currently calls lib.mkGitHooks
with an empty attribute set which uses defaults; update the
documentation/comments near the mkGitHooks invocation to note that mkGitHooks
supports customization via extraHooks and rustToolchain (see lib/hooks.nix) so
downstream consumers know they can pass those keys (e.g., extraHooks = ... or
rustToolchain = ...) to customize hooks and Rust toolchain behavior; include a
short example in the comment showing the attribute names and purpose.
github-merge-queue
Bot
removed this pull request from the merge queue due to no response for status checks
There was a problem hiding this comment.
♻️ Duplicate comments (3)
flake.nix (2)
111-113:⚠️ Potential issue | 🟠 MajorDon't
throwfrompackages.diskoon unsupported systems.
eachDefaultSystempublishes outputs for platforms that the pinneddiskoinput may not support. This fallback turns a missing upstream package into a full flake-evaluation failure instead of simply omittingdiskofrompackages.♻️ Suggested fix
- packages.disko = - disko.packages.${system}.default or (throw "disko package not available for ${system}"); - packages.ragenix = ragenix.packages.${system}.default; + packages = + { + ragenix = ragenix.packages.${system}.default; + } + // pkgs.lib.optionalAttrs (builtins.hasAttr system disko.packages) { + disko = disko.packages.${system}.default; + };To verify against the pinned
diskorevision, the script below prints the revision fromflake.lockand the upstream files that declare supported systems. If one of the defaultflake-utilssystems is absent there, the currentthrowis the blocker on that platform.#!/bin/bash set -euo pipefail disko_rev=$(python - <<'PY' import json with open("flake.lock") as f: lock = json.load(f) print(lock["nodes"]["disko"]["locked"]["rev"]) PY ) echo "Pinned disko revision: ${disko_rev}" echo echo "Current package block:" sed -n '104,113p' flake.nix echo for file in flake.nix default.nix package.nix; do echo "== ${file} @ ${disko_rev} ==" curl -Lsf "https://raw.githubusercontent.com/nix-community/disko/${disko_rev}/${file}" \ | rg -n 'supportedSystems|each(Default)?System|x86_64-linux|aarch64-linux|x86_64-darwin|aarch64-darwin|packages|default' || true echo done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flake.nix` around lines 111 - 113, The packages.disko entry currently does "disko.packages.${system}.default or (throw ...)" which causes full flake evaluation failures on systems that the pinned disko input doesn't support; change that fallback to a non-fatal value (for example replace the throw with null or remove the attribute via builtins.getAttr or getAttrDefault) so unsupported systems simply omit disko from packages when using eachDefaultSystem — update the expression referencing packages.disko (and keep packages.ragenix unchanged) to return null/absent instead of throwing.
108-109: 🧹 Nitpick | 🔵 TrivialKeep the dev-shell tool list sourced from the hook definition.
lib/hooks.nixalready owns the default formatter/linter set, but the same tools are hardcoded again here. That will drift the first timeextraHooksorrustToolchainchanges the hook configuration, so please derive the dev-shell packages from the same source of truth instead of maintaining both lists.Also applies to: 122-129
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flake.nix` around lines 108 - 109, The dev-shell tool list is hardcoded instead of being derived from the git hooks configuration; replace the duplicated tool list with the set produced by the hook definition by assigning hooks = self.lib.mkGitHooks { ... } (including extraHooks and rustToolchain) and then reference the resulting hooks' tool list when building the dev-shell. Concretely, stop re-declaring the formatter/linter packages and consume the packages exported by the result of self.lib.mkGitHooks (the same `hooks` value used here and in the repeated block at 122-129) to populate the dev-shell tools so changes to extraHooks or rustToolchain flow through automatically..github/workflows/ci.yaml (1)
15-17:⚠️ Potential issue | 🟠 MajorPin every action to an immutable commit SHA.
These
uses:refs are still mutable tags, so an upstream retag can change CI behavior without a PR here. Replace them with full commit SHAs and keep the tag in a trailing comment if you want readability.To resolve the exact immutable refs, the script below prints the commit behind each current tag. The expected result is one full SHA per action; those SHAs should replace the tag refs in both jobs.
#!/bin/bash set -euo pipefail for spec in \ "actions/checkout v4" \ "DeterminateSystems/nix-installer-action v17" \ "DeterminateSystems/magic-nix-cache-action v8" do repo=${spec% *} tag=${spec#* } sha=$(git ls-remote "https://github.com/${repo}.git" "refs/tags/${tag}^{}" | awk '{print $1}') if [ -z "$sha" ]; then sha=$(git ls-remote "https://github.com/${repo}.git" "refs/tags/${tag}" | awk '{print $1}') fi printf '%s@%s -> %s\n' "$repo" "$tag" "$sha" done echo echo "Current workflow refs:" sed -n '12,33p' .github/workflows/ci.yamlAlso applies to: 31-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yaml around lines 15 - 17, Replace the mutable action tags with immutable commit SHAs for each `uses:` entry — specifically change `actions/checkout@v4`, `DeterminateSystems/nix-installer-action@v17`, and `DeterminateSystems/magic-nix-cache-action@v8` to the full commit SHA for the tag (you can use the provided script to lookup the SHAs), and keep the original tag as a trailing comment for readability; make the same replacements for the duplicate occurrences of those three `uses:` refs elsewhere in the workflow so all refs are pinned to immutable commits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/ci.yaml:
- Around line 15-17: Replace the mutable action tags with immutable commit SHAs
for each `uses:` entry — specifically change `actions/checkout@v4`,
`DeterminateSystems/nix-installer-action@v17`, and
`DeterminateSystems/magic-nix-cache-action@v8` to the full commit SHA for the
tag (you can use the provided script to lookup the SHAs), and keep the original
tag as a trailing comment for readability; make the same replacements for the
duplicate occurrences of those three `uses:` refs elsewhere in the workflow so
all refs are pinned to immutable commits.
In `@flake.nix`:
- Around line 111-113: The packages.disko entry currently does
"disko.packages.${system}.default or (throw ...)" which causes full flake
evaluation failures on systems that the pinned disko input doesn't support;
change that fallback to a non-fatal value (for example replace the throw with
null or remove the attribute via builtins.getAttr or getAttrDefault) so
unsupported systems simply omit disko from packages when using eachDefaultSystem
— update the expression referencing packages.disko (and keep packages.ragenix
unchanged) to return null/absent instead of throwing.
- Around line 108-109: The dev-shell tool list is hardcoded instead of being
derived from the git hooks configuration; replace the duplicated tool list with
the set produced by the hook definition by assigning hooks = self.lib.mkGitHooks
{ ... } (including extraHooks and rustToolchain) and then reference the
resulting hooks' tool list when building the dev-shell. Concretely, stop
re-declaring the formatter/linter packages and consume the packages exported by
the result of self.lib.mkGitHooks (the same `hooks` value used here and in the
repeated block at 122-129) to populate the dev-shell tools so changes to
extraHooks or rustToolchain flow through automatically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9940b931-a594-42fd-89ea-defe2c180683
📒 Files selected for processing (2)
.github/workflows/ci.yamlflake.nix
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.nix
📄 CodeRabbit inference engine (AGENTS.md)
**/*.nix: Uselib.mkOptionwith proper types for all module options
Every module must be independently usable (no implicit dependencies on other omnix modules)
Options must live under theomnix.*namespace
Uselib.mkIf cfg.enablefor conditional configuration
Uselib.mkDefaultfor overridable defaults
Every configurable value must be a module option with a type and description
Preferlib.types.stroverlib.types.pathfor values that may come from terraform or other dynamic sources
Group related options under a singleomnix.<module>prefix
Functions take an attrset of required parameters
Return an attrset of derivations or values from library functions
Usepkgs.writeShellApplicationfor shell scripts (not rawwriteScript)
IncluderuntimeInputsin shell scripts and never assume tools are on PATH
All shell scripts must useset -eo pipefail
When changing module option interfaces, preserve the old option withlib.mkRenamedOptionModuleorlib.mkRemovedOptionModulewith a clear migration message
Files:
flake.nix
*
⚙️ CodeRabbit configuration file
Focus on providing constructive criticism. Whenever you see a suboptimal approach, suggest more idiomatic or robust alternative(s). Flag potential footguns. Suggest FP alternatives to mutable/imperative code. Point out architectural flaws like leaky abstractions, tight coupling, wrong level of abstraction, poor type modeling, over-abstraction, unclear domain boundaries. Code should generally be organized based on business concerns rather than technical aspects - suggest improvements if you find violations. Point out gaps in test coverage but suggest tests that are not too coupled to the implementation and actually test domain invariants and business logic
Files:
flake.nix
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Use `pkgs.writeShellApplication` for shell scripts (not raw `writeScript`)
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Include `runtimeInputs` in shell scripts and never assume tools are on PATH
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Template projects should pass `nix flake check`
Applied to files:
.github/workflows/ci.yamlflake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Consumer repos using devenv dev shells require `nix flake check --impure` (devenv needs impure evaluation to resolve the working directory)
Applied to files:
flake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Include `runtimeInputs` in shell scripts and never assume tools are on PATH
Applied to files:
flake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Use `pkgs.writeShellApplication` for shell scripts (not raw `writeScript`)
Applied to files:
flake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Use `lib.mkDefault` for overridable defaults
Applied to files:
flake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Use `lib.mkOption` with proper types for all module options
Applied to files:
flake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Every module must be independently usable (no implicit dependencies on other omnix modules)
Applied to files:
flake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Return an attrset of derivations or values from library functions
Applied to files:
flake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Group related options under a single `omnix.<module>` prefix
Applied to files:
flake.nix
📚 Learning: 2026-03-21T15:47:14.585Z
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Applies to **/*.nix : Options must live under the `omnix.*` namespace
Applied to files:
flake.nix
🔇 Additional comments (2)
.github/workflows/ci.yaml (1)
23-28: Good public-surface CI coverage.Evaluating and dry-running the exported
nixosConfigurations.<name>.config.system.build.toplevelkeeps this job aligned with the flake interface consumers actually use.Also applies to: 35-39
flake.nix (1)
37-61: Nice to expose the examples as first-class flake outputs.These give CI and downstream users stable public entry points to validate instead of reaching into individual files.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
.github/workflows/infra.yaml (2)
98-102:⚠️ Potential issue | 🟠 MajorHarden all SSH calls to be non-interactive and time-bounded.
The wait loop still lacks
BatchMode=yes, and the later verification SSH commands (Line 120 and Line 124) drop timeout/non-interactive options entirely. This can stall jobs on auth or network edge cases.Suggested change
- name: Wait for SSH run: | retries=0 - until ssh -o StrictHostKeyChecking=no -o ConnectTimeout=5 \ + until ssh -o StrictHostKeyChecking=no -o ConnectTimeout=5 -o BatchMode=yes \ -i ~/.ssh/id_ed25519 "root@$HOST_IP" true 2>/dev/null; do retries=$((retries + 1)) if [ "$retries" -ge 30 ]; then echo "SSH not available after 2.5 minutes" >&2 exit 1 fi sleep 5 done - name: Verify SSH access - run: ssh -i ~/.ssh/id_ed25519 "root@$HOST_IP" uname -a + run: ssh -o StrictHostKeyChecking=no -o ConnectTimeout=5 -o BatchMode=yes \ + -i ~/.ssh/id_ed25519 "root@$HOST_IP" uname -a - name: Verify NixOS run: | - RESULT=$(ssh -i ~/.ssh/id_ed25519 "root@$HOST_IP" \ + RESULT=$(ssh -o StrictHostKeyChecking=no -o ConnectTimeout=5 -o BatchMode=yes \ + -i ~/.ssh/id_ed25519 "root@$HOST_IP" \ "grep -c NixOS /etc/os-release") if [ "$RESULT" -lt 1 ]; then echo "Host is not running NixOS" >&2 exit 1 fiAlso applies to: 119-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/infra.yaml around lines 98 - 102, The SSH wait loop and subsequent verification SSH invocations are interactive and risk hanging; update every ssh invocation (the wait loop command `ssh -o StrictHostKeyChecking=no -o ConnectTimeout=5 -i ~/.ssh/id_ed25519 "root@$HOST_IP" true` and the later verification SSH commands) to include `-o BatchMode=yes` and an explicit `-o ConnectTimeout=<seconds>` (and preserve `-o StrictHostKeyChecking=no` and the identity `-i ~/.ssh/id_ed25519`) so all SSH calls are non-interactive and time-bounded; ensure you apply the same set of SSH options to each SSH call in the workflow.
131-134:⚠️ Potential issue | 🟠 MajorDo not suppress
tfDestroyfailures.Line 134 hides cleanup errors with
|| true, which can leave resources alive while the job appears successful.Suggested change
- run: nix run .#tfDestroy -- -i ~/.ssh/id_ed25519 || true + run: nix run .#tfDestroy -- -i ~/.ssh/id_ed25519🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/infra.yaml around lines 131 - 134, The Teardown step currently suppresses tfDestroy failures by appending "|| true" to the run command; remove that "|| true" so the "nix run .#tfDestroy -- -i ~/.ssh/id_ed25519" command can fail the workflow when destroy fails (leave the step name "Teardown" and the run invocation "nix run .#tfDestroy" intact so failures are visible and resources aren't left alive silently).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/infra.yaml:
- Around line 58-59: Replace the hostile TODO comment text ("get rid of this
retarded garbage...") with a neutral, professional note that explains the issue
and suggests a corrective action; for example, change the line to something like
"TODO: refactor this to use workflow variables or other standard approaches to
improve readability and maintainability" (i.e., remove profanity, state the
problem, and recommend using variables/standard patterns).
- Around line 84-90: Add a new workflow step to run a flake validation before
running Terraform: insert a step (e.g., name: "Nix flake check") before the
"Terraform init and apply" step that runs in the same working-directory ${{
env.TEST_DIR }} and executes either `nix flake check` or `nix run .#flakeCheck`
(so the pipeline fails fast on template breakage); ensure this step runs with
the same environment/SSH setup as the subsequent tfInit/tfPlan/tfApply steps and
returns non-zero on failure so provisioning is blocked when the flake check
fails.
- Around line 4-5: The workflow currently triggers on pull_request_review types:
[submitted] which runs for all review submissions; update the job to only
proceed for approved reviews by adding a job-level condition that checks
github.event.review.state == 'approved' (i.e., add if: github.event.review.state
== 'approved' on the job that runs the expensive infra lifecycle). Target the
pull_request_review event and the job(s) that depend on that event so only
reviews with state "approved" execute the full lifecycle.
---
Duplicate comments:
In @.github/workflows/infra.yaml:
- Around line 98-102: The SSH wait loop and subsequent verification SSH
invocations are interactive and risk hanging; update every ssh invocation (the
wait loop command `ssh -o StrictHostKeyChecking=no -o ConnectTimeout=5 -i
~/.ssh/id_ed25519 "root@$HOST_IP" true` and the later verification SSH commands)
to include `-o BatchMode=yes` and an explicit `-o ConnectTimeout=<seconds>` (and
preserve `-o StrictHostKeyChecking=no` and the identity `-i ~/.ssh/id_ed25519`)
so all SSH calls are non-interactive and time-bounded; ensure you apply the same
set of SSH options to each SSH call in the workflow.
- Around line 131-134: The Teardown step currently suppresses tfDestroy failures
by appending "|| true" to the run command; remove that "|| true" so the "nix run
.#tfDestroy -- -i ~/.ssh/id_ed25519" command can fail the workflow when destroy
fails (leave the step name "Teardown" and the run invocation "nix run
.#tfDestroy" intact so failures are visible and resources aren't left alive
silently).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7aee4467-f2c6-402e-8029-81a964298d61
📒 Files selected for processing (1)
.github/workflows/infra.yaml
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: data-cartel/omnix PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-21T15:47:14.585Z
Learning: Template projects should pass `nix flake check`
| pull_request_review: | ||
| types: [submitted] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🌐 Web query:
For GitHub Actions pull_request_reviewevents, what are the possiblegithub.event.review.statevalues, and istypes: [submitted] triggered for both approval and comment-only reviews?
💡 Result:
The possible values for github.event.review.state in GitHub Actions pull_request_review events are: APPROVED, CHANGES_REQUESTED, COMMENTED, DISMISSED, and PENDING. Yes, types: [submitted] is triggered for both approval (APPROVED) and comment-only (COMMENTED) reviews, as "submitted" refers to the pull_request_review event being triggered when a review is submitted (i.e., state changes from PENDING to one of the submitted states: APPROVED, CHANGES_REQUESTED, or COMMENTED). PENDING reviews do not trigger the event, and DISMISSED may trigger on dismissed type.
Citations:
- 1: https://docs.github.com/actions/using-workflows/events-that-trigger-workflows
- 2: https://docs.github.com/en/rest/pulls/reviews
- 3: https://stackoverflow.com/questions/49145734/what-are-all-possible-github-pull-request-review-statuses
- 4: Parse 'reviewed' event object 'state' property google/go-github#1886
- 5: Undocumented event "PullRequestReviewEvent" github/docs#2709
- 6: https://docs.github.com/rest/pulls/reviews
🏁 Script executed:
#!/bin/bash
# Locate and examine the infra.yaml workflow file
fd -i "infra.yaml" -type fRepository: data-cartel/omnix
Length of output: 231
🏁 Script executed:
#!/bin/bash
# Find infra.yaml workflow file
fd "infra.yaml" .github/workflows/Repository: data-cartel/omnix
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Read the infra.yaml file
cat -n .github/workflows/infra.yamlRepository: data-cartel/omnix
Length of output: 5024
🌐 Web query:
GitHub Actions github.event.review.state case sensitivity uppercase or lowercase comparison
💡 Result:
In GitHub Actions, comparisons with github.event.review.state are case-sensitive. The value is lowercase, such as 'approved', 'changes_requested', or 'commented'. Using uppercase like 'APPROVED' will not match. Official GitHub docs show examples using lowercase: if: github.event.review.state == 'approved' (result 7 from first search). A 2023 PR in github/docs corrected documentation from lowercase to uppercase 'APPROVED' because the example didn't work, confirming the actual value is uppercase in some contexts like GraphQL, but for Actions webhook payloads, it's lowercase as per working examples and API consistency (results 1, 6, 7). REST API review states are uppercase (APPROVED, etc., result 8, 19), but webhook payloads for pull_request_review events use lowercase for the review.state field, as evidenced by multiple code examples and the correction needed in docs.
Citations:
- 1: Correct case of github.event.review.state github/docs#28303
- 2: https://stackoverflow.com/questions/69493260/get-approval-status-in-github-action
- 3: https://docs.github.com/actions/using-workflows/events-that-trigger-workflows
- 4: https://docs.github.com/en/rest/pulls/reviews
- 5: https://docs.github.com/rest/pulls/reviews
Gate infra runs to approved reviews only.
pull_request_review with types: [submitted] runs this expensive full lifecycle on comment-only and change-request reviews as well. Gate the job to approved reviews only:
Suggested change
jobs:
infra:
name: Full lifecycle
+ if: github.event_name != 'pull_request_review' || github.event.review.state == 'approved'
runs-on: ubuntu-latest
timeout-minutes: 30🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/infra.yaml around lines 4 - 5, The workflow currently
triggers on pull_request_review types: [submitted] which runs for all review
submissions; update the job to only proceed for approved reviews by adding a
job-level condition that checks github.event.review.state == 'approved' (i.e.,
add if: github.event.review.state == 'approved' on the job that runs the
expensive infra lifecycle). Target the pull_request_review event and the job(s)
that depend on that event so only reviews with state "approved" execute the full
lifecycle.
| # TODO: get rid of this retarded garbage and learn the concept of variables | ||
| # there is like a thousand ways of doing this and this is by far the dumbest |
There was a problem hiding this comment.
Replace hostile TODO wording with neutral guidance.
The current comment text is unprofessional and makes maintenance harder for collaborators.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/infra.yaml around lines 58 - 59, Replace the hostile TODO
comment text ("get rid of this retarded garbage...") with a neutral,
professional note that explains the issue and suggests a corrective action; for
example, change the line to something like "TODO: refactor this to use workflow
variables or other standard approaches to improve readability and
maintainability" (i.e., remove profanity, state the problem, and recommend using
variables/standard patterns).
| - name: Terraform init and apply | ||
| working-directory: ${{ env.TEST_DIR }} | ||
| run: | | ||
| nix run .#tfInit -- -i ~/.ssh/id_ed25519 | ||
| nix run .#tfPlan -- -i ~/.ssh/id_ed25519 | ||
| nix run .#tfApply -- -i ~/.ssh/id_ed25519 | ||
|
|
There was a problem hiding this comment.
Run nix flake check before cloud provisioning.
The workflow goes straight to tfInit/tfPlan/tfApply without validating the generated template flake, so basic template breakage is discovered only after infra actions begin.
Suggested change
- name: Terraform init and apply
working-directory: ${{ env.TEST_DIR }}
run: |
+ nix flake check
nix run .#tfInit -- -i ~/.ssh/id_ed25519
nix run .#tfPlan -- -i ~/.ssh/id_ed25519
nix run .#tfApply -- -i ~/.ssh/id_ed25519Based on learnings: Template projects should pass nix flake check.
📝 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.
| - name: Terraform init and apply | |
| working-directory: ${{ env.TEST_DIR }} | |
| run: | | |
| nix run .#tfInit -- -i ~/.ssh/id_ed25519 | |
| nix run .#tfPlan -- -i ~/.ssh/id_ed25519 | |
| nix run .#tfApply -- -i ~/.ssh/id_ed25519 | |
| - name: Terraform init and apply | |
| working-directory: ${{ env.TEST_DIR }} | |
| run: | | |
| nix flake check | |
| nix run .#tfInit -- -i ~/.ssh/id_ed25519 | |
| nix run .#tfPlan -- -i ~/.ssh/id_ed25519 | |
| nix run .#tfApply -- -i ~/.ssh/id_ed25519 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/infra.yaml around lines 84 - 90, Add a new workflow step
to run a flake validation before running Terraform: insert a step (e.g., name:
"Nix flake check") before the "Terraform init and apply" step that runs in the
same working-directory ${{ env.TEST_DIR }} and executes either `nix flake check`
or `nix run .#flakeCheck` (so the pipeline fails fast on template breakage);
ensure this step runs with the same environment/SSH setup as the subsequent
tfInit/tfPlan/tfApply steps and returns non-zero on failure so provisioning is
blocked when the flake check fails.
2 participants
Summary by CodeRabbit
New Features
Documentation
Improvements