feat: add Nix development environment with flake support #1119
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a Nix flake ( Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer (shell)
participant Direnv as direnv
participant Flake as flake.nix
participant Shell as devShell (shellHook)
Dev->>Direnv: enter project directory
Direnv->>Flake: "use flake" — request flake activation
Flake->>Flake: resolve supportedSystems & select host attrs
Flake->>Shell: instantiate devShell.default for host
Shell->>Dev: run shellHook — set GOPATH/GOBIN/GOMODCACHE/GOCACHE, create bin dirs, set npm prefix, update PATH
note right of Shell: dev tooling and per-project bins prepared
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
Comment |
danpiths
changed the base branch from
12-08-feat_governance_plugin_refactor
to
graphite-base/1119
danpiths
force-pushed
the
feature/12-12-_nix_support_adds_nix_flake_support_for_bifrost
branch
from
bc0a584 to
0ed2f53
Compare
danpiths
changed the base branch from
graphite-base/1119
to
12-09-chore_governance_tests_added
danpiths
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Makefile (1)
154-155: Consider clarifying the static build message.The sentence now reads: "This will create a statically linked build, to build with dynamic plugin support." This is grammatically awkward since static linking doesn't support dynamic plugins. Compare with the dynamic branch (lines 151-152) which has clearer separate sentences.
Consider keeping the period and capitalization to match the dynamic branch, or rewording to clarify that users should use
DYNAMIC=1for plugin support:- echo "$(YELLOW)Note: This will create a statically linked build,$(NC)"; \ - echo "$(YELLOW)to build with dynamic plugin support.$(NC)"; \ + echo "$(YELLOW)Note: This will create a statically linked build.$(NC)"; \ + echo "$(YELLOW)Use DYNAMIC=1 for dynamic plugin support.$(NC)"; \flake.nix (2)
12-16: Consider adding x86_64-darwin support for Intel Macs.The supported systems list omits
x86_64-darwin(64-bit Intel macOS). While Apple Silicon is increasingly common, some developers may still use Intel Macs.supportedSystems = [ "x86_64-linux" # 64-bit Intel/AMD Linux "aarch64-linux" # 64-bit ARM Linux "aarch64-darwin" # 64-bit ARM macOS + "x86_64-darwin" # 64-bit Intel macOS ];
68-68: Prepend project-local paths to PATH for tool priority.Appending to PATH means system-wide tools take precedence over project-local ones. Prepending ensures the Nix-managed project tools are used first:
- export PATH="$PATH:$GOBIN" + export PATH="$GOBIN:$PATH"Apply the same pattern to line 97:
- export PATH="$PATH:$npm_config_prefix/bin" + export PATH="$npm_config_prefix/bin:$PATH"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.envrc(1 hunks).gitignore(1 hunks)Makefile(2 hunks)flake.nix(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
flake.nixMakefile
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (4)
.envrc (1)
1-1: LGTM!Standard direnv configuration for Nix flake integration. This correctly enables automatic environment activation when entering the project directory.
.gitignore (1)
58-60: LGTM!The ignore patterns correctly exclude the direnv cache directory and the project-local
.nix-storedirectory used for Go and npm artifacts in the flake's shellHook.flake.nix (2)
70-85: LGTM with minor consideration.The auto-installation of Go tools (gopls, air, dlv, staticcheck) is convenient for onboarding. The
command -vchecks ensure tools are only installed if missing, which is efficient for subsequent shell activations.Note: First-time shell activation will be slower due to tool downloads. This is acceptable for a development environment.
48-51:nodejs_24is available in nixpkgs-unstable.Node.js 24 is included in nixos-unstable, and Node.js 24 is in active LTS status and ready for production use. There's no need to change this reference unless targeting macOS, where there are known build issues.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Makefile (1)
154-156: Clarify the build message for static linking.The current message reads: "This will create a statically linked build, to build with dynamic plugin support." This phrasing is incomplete—it seems to be trailing off without explaining how to build with dynamic plugin support. Consider completing the sentence:
else \ - echo "$(YELLOW)Note: This will create a statically linked build,$(NC)"; \ - echo "$(YELLOW)to build with dynamic plugin support.$(NC)"; \ + echo "$(YELLOW)Note: This will create a statically linked build.$(NC)"; \ + echo "$(YELLOW)To build with dynamic plugin support, use: make build DYNAMIC=1$(NC)"; \ fiflake.nix (2)
12-16: Consider adding x86_64-darwin support.Intel-based Macs (x86_64-darwin) are not included in the supported systems. While Apple Silicon is increasingly common, some developers may still use Intel Macs.
supportedSystems = [ "x86_64-linux" # 64-bit Intel/AMD Linux "aarch64-linux" # 64-bit ARM Linux "aarch64-darwin" # 64-bit ARM macOS + "x86_64-darwin" # 64-bit Intel macOS ];
68-68: Consider prepending project paths to PATH.The current configuration appends
$GOBINafter$PATH, meaning system-installed tools take precedence over project-local ones. For a reproducible dev environment, project-local tools should typically take precedence.- export PATH="$PATH:$GOBIN" + export PATH="$GOBIN:$PATH"Apply the same pattern on line 97 for npm:
- export PATH="$PATH:$npm_config_prefix/bin" + export PATH="$npm_config_prefix/bin:$PATH"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.envrc(1 hunks).gitignore(1 hunks)Makefile(2 hunks)flake.nix(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
Makefileflake.nix
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (4)
.envrc (1)
1-1: LGTM!Standard direnv configuration for Nix flake integration. The
use flakedirective correctly references theflake.nixin the same directory, and direnv's security model requires explicitdirenv allowbefore activation..gitignore (1)
58-60: LGTM!The Nix-specific ignores correctly match the directories configured in
flake.nix:.direnvfor direnv cache and.nix-storefor the project-local Go/npm artifacts defined in the shellHook.flake.nix (2)
70-85: Tool installation runs on every shell entry when tools are missing.The conditional installation is correct, but note that
go installwill run during shell activation if tools are missing, which may cause a delay on first entry or if GOBIN is cleared. This is acceptable but worth documenting in the PR description or README for developer awareness.
48-51: Verifynodejs_24package availability in current nixpkgs.The package naming convention
nodejs_24aligns with nixpkgs patterns (e.g.,nodejs_22,nodejs_23), but availability in nixpkgs-unstable could not be confirmed. Check the nixpkgs package repository or usenix search nixpkgs nodejsto verify the exact package name available for your target Node.js version.
Pratham-Mishra04
changed the base branch from
12-09-chore_governance_tests_added
to
graphite-base/1119
danpiths
force-pushed
the
feature/12-12-_nix_support_adds_nix_flake_support_for_bifrost
branch
from
0ed2f53 to
b9764ab
Compare
danpiths
force-pushed
the
graphite-base/1119
branch
from
e0d915d to
b15b300
Compare
danpiths
changed the base branch from
graphite-base/1119
to
12-09-chore_governance_tests_added
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
flake.nix (1)
12-16: Consider adding x86_64-darwin support for Intel Mac users.The flake supports ARM macOS (aarch64-darwin) but omits x86_64-darwin (Intel macOS). If any developers use Intel Macs, they won't be able to use this development environment. If this is intentional, no action needed.
🔎 Proposed addition
supportedSystems = [ "x86_64-linux" # 64-bit Intel/AMD Linux "aarch64-linux" # 64-bit ARM Linux "aarch64-darwin" # 64-bit ARM macOS + "x86_64-darwin" # 64-bit Intel macOS ];
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.envrc.gitignoreMakefileflake.nix
🚧 Files skipped from review as they are similar to previous changes (3)
- .envrc
- Makefile
- .gitignore
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
flake.nix
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (4)
flake.nix (4)
27-28: Verify that allowUnfree is necessary.The configuration enables unfree (proprietary) packages. The currently listed packages (go, nodejs_24) are both free/open-source. If you don't need any proprietary tools, you can remove this setting for stricter licensing compliance.
48-51: Package declarations look good.The base packages (go, nodejs_24) are appropriate. However, the Go development tools installed later in the shellHook (gopls, air, dlv, staticcheck) should ideally be declared here as Nix packages for better reproducibility. This will be addressed in a separate comment on the shellHook section.
87-97: Node/npm environment setup looks good.The npm configuration appropriately sets up a project-local "global" prefix, which allows developers to install global npm packages in an isolated project directory. This approach is idiomatic and doesn't have the reproducibility issues present in the Go setup.
6-6: Use a stable nixpkgs branch instead of staging-next.The staging-next branch is intended only for stabilization fixes and security fixes before merging to master, and receives mass rebuilds like Mesa updates, introducing instability. For a reproducible development environment, use nixpkgs-unstable which follows the master branch delivering tested updates, or pin a stable release like
nixos-24.11.- nixpkgs.url = "github:NixOS/nixpkgs/staging-next"; + nixpkgs.url = "github:NixOS/nixpkgs/nixpkgs-unstable";
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
flake.nix (3)
54-54: Optional: Remove redundant emptyenvobject.The empty
env = {}object can be removed as it serves no purpose.🔎 Proposed change
- # Set any environment variables for your development environment - env = {}; - # Add any shell logic you want executed when the environment is activated
70-85: Consider using Nix packages for Go tools instead of@latestinstallations.Installing Go tools with
@latestundermines the reproducibility that Nix provides. Different developers entering the environment at different times will get different tool versions, and the environment won't be truly reproducible.Consider adding these tools to the
packageslist instead:🔎 Alternative approach using Nix packages
packages = with pkgs; [ go nodejs_24 + gopls + air + delve + go-tools # provides staticcheck ];This approach ensures:
- All developers get the exact same tool versions
- Tools are cached by Nix and don't need to be compiled
- The environment is truly reproducible
- Faster shell activation (no need to check and install)
If you need to keep the shellHook approach, consider pinning specific versions instead of using
@latest:go install golang.org/x/tools/[email protected]
68-68: Consider prepending to PATH instead of appending.Lines 68 and 97 append the project-local bin directories to
$PATH. Prepending (e.g.,export PATH="$GOBIN:$PATH") ensures that project-local tools take precedence over system-wide installations, which is typically the desired behavior in a development environment.🔎 Proposed change
- export PATH="$PATH:$GOBIN" + export PATH="$GOBIN:$PATH"- export PATH="$PATH:$npm_config_prefix/bin" + export PATH="$npm_config_prefix/bin:$PATH"Also applies to: 97-97
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.envrc.gitignoreMakefileflake.nix
✅ Files skipped from review due to trivial changes (1)
- Makefile
🚧 Files skipped from review as they are similar to previous changes (2)
- .envrc
- .gitignore
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
flake.nix
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
🔇 Additional comments (1)
flake.nix (1)
12-16: Consider adding Intel macOS support.The flake currently supports ARM macOS (
aarch64-darwin) but not Intel macOS (x86_64-darwin). If any team members use Intel Macs, they won't be able to use this development environment.Consider adding
"x86_64-darwin"to thesupportedSystemslist if Intel Mac support is needed:🔎 Proposed addition
supportedSystems = [ "x86_64-linux" # 64-bit Intel/AMD Linux "aarch64-linux" # 64-bit ARM Linux "aarch64-darwin" # 64-bit ARM macOS + "x86_64-darwin" # 64-bit Intel macOS ];
danpiths
force-pushed
the
feature/12-12-_nix_support_adds_nix_flake_support_for_bifrost
branch
from
b9764ab to
f6b85fa
Compare
Pratham-Mishra04
changed the base branch from
12-09-chore_governance_tests_added
to
graphite-base/1119
danpiths
force-pushed
the
feature/12-12-_nix_support_adds_nix_flake_support_for_bifrost
branch
from
f6b85fa to
654de5d
Compare
danpiths
force-pushed
the
graphite-base/1119
branch
from
b15b300 to
4f8ebad
Compare
danpiths
changed the base branch from
graphite-base/1119
to
12-09-chore_governance_tests_added
Pratham-Mishra04
changed the base branch from
12-09-chore_governance_tests_added
to
graphite-base/1119
danpiths
force-pushed
the
feature/12-12-_nix_support_adds_nix_flake_support_for_bifrost
branch
from
654de5d to
be49a44
Compare
danpiths
force-pushed
the
graphite-base/1119
branch
from
4f8ebad to
0753937
Compare
akshaydeo
force-pushed
the
feature/12-12-_nix_support_adds_nix_flake_support_for_bifrost
branch
from
be49a44 to
ee064c2
Compare
akshaydeo
force-pushed
the
feature/12-12-_nix_support_adds_nix_flake_support_for_bifrost
branch
from
ee064c2 to
51556a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.gitignore (1)
43-45: Consolidate duplicate "Cursor specific" section headers.Two separate "Cursor specific" comment sections exist at lines 43-45 (containing
.claude) and lines 55-56 (containing.cursor/). Consolidate these into a single section for better maintainability and clarity.🔎 Proposed consolidation
# Cursor specific .claude +.cursor/ -# Python specific +# Python specificThen remove the duplicate section header at lines 55-56.
Also applies to: 55-56
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.envrc.gitignoreMakefileflake.nix
✅ Files skipped from review due to trivial changes (1)
- .envrc
🚧 Files skipped from review as they are similar to previous changes (2)
- flake.nix
- Makefile
🔇 Additional comments (1)
.gitignore (1)
58-60: Nix-specific entries align well with the flake-based dev environment.The addition of
.direnvand.nix-storeto the ignore list is appropriate for the Nix Flakes integration. These entries ensure that direnv and Nix store artifacts are not accidentally committed.
danpiths
force-pushed
the
feature/12-12-_nix_support_adds_nix_flake_support_for_bifrost
branch
2 times, most recently
from
c6e01b1 to
60af306
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
flake.nix (2)
6-6: Consider documenting the staging-next branch choice.As noted in previous discussions,
staging-nextis used because Go 1.25.5 is required and not yet available onnixpkgs-unstable. Adding a brief inline comment would help future maintainers understand when it's safe to migrate to a more stable branch.🔎 Suggested documentation addition
inputs = { + # Using staging-next for Go 1.25.5 availability (not yet in nixpkgs-unstable) nixpkgs.url = "github:NixOS/nixpkgs/staging-next"; };
74-74: Consider prepending project tool directories to PATH instead of appending.The shellHook appends
$GOBINand$npm_config_prefix/bintoPATH, which means system-installed tools will take precedence over the Nix-provided project tools. This could lead to unexpected behavior if system-installed versions differ from those in the Nix environment.🔎 Suggested PATH precedence change
mkdir -p "$GOBIN" "$GOMODCACHE" "$GOCACHE" - export PATH="$PATH:$GOBIN" + export PATH="$GOBIN:$PATH" ## ## Node: project-local "global" npm prefixmkdir -p "$npm_config_prefix/bin" # Ensure "global" npm bin is on PATH for this shell only - export PATH="$PATH:$npm_config_prefix/bin" + export PATH="$npm_config_prefix/bin:$PATH" '';Alternatively, consolidate both PATH updates into a single export:
mkdir -p "$GOBIN" "$GOMODCACHE" "$GOCACHE" - export PATH="$PATH:$GOBIN" - ## ## Node: project-local "global" npm prefix ##mkdir -p "$npm_config_prefix/bin" - # Ensure "global" npm bin is on PATH for this shell only - export PATH="$PATH:$npm_config_prefix/bin" + # Ensure project tools take precedence over system tools + export PATH="$GOBIN:$npm_config_prefix/bin:$PATH" '';Also applies to: 86-86
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.envrc.gitignoreMakefileflake.nix
✅ Files skipped from review due to trivial changes (1)
- Makefile
🚧 Files skipped from review as they are similar to previous changes (1)
- .envrc
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
flake.nix
🔇 Additional comments (4)
.gitignore (1)
56-60: LGTM! Appropriate ignore rules for the Nix development environment.The updates correctly ignore Nix-specific artifacts (
.direnvfrom direnv activation and.nix-storeused as the project-local directory in the flake's shellHook) and switch.cursor/from negated to actively ignored.flake.nix (3)
28-28: Verify that allowUnfree is necessary for the development environment.The flake enables unfree packages with
config.allowUnfree = true. Based on the packages declared (Go, Node.js, and Go development tools), all appear to be open-source and freely licensed. Enabling unfree packages may be unnecessary unless specific unfree dependencies are planned.If unfree packages aren't required, consider removing this setting to maintain a more restrictive (and typical) Nix environment:
🔎 Suggested change if unfree packages aren't needed
pkgs = import inputs.nixpkgs { inherit system; - # Enable using unfree packages - config.allowUnfree = true; };
48-57: LGTM! Go tools are now declared as reproducible Nix packages.The development tools (gopls, air, delve, go-tools, gofumpt) are correctly declared as Nix packages rather than being installed dynamically with
@latest. This ensures reproducible builds across all developers and addresses the concern from the previous review.
12-16: Document the rationale for excluding x86_64-darwin (Intel macOS) support.The flake.nix explicitly supports ARM macOS but not Intel macOS. While this likely reflects Apple's deprecation of Intel Macs in favor of Apple Silicon, the decision should be documented if intentional. If any team members still use Intel-based Macs, add
"x86_64-darwin"to the supportedSystems list.
danpiths
force-pushed
the
feature/12-12-_nix_support_adds_nix_flake_support_for_bifrost
branch
from
60af306 to
d4e7365
Compare
danpiths
changed the base branch from
graphite-base/1119
to
11-19-plugins_v2_architecture
danpiths
changed the base branch from
11-19-plugins_v2_architecture
to
graphite-base/1119
Summary
Add Nix development environment support to the project using Nix Flakes,
providing a consistent and reproducible development environment across different
platforms.
Changes
flake.nixto define the development environment with Go and Node.jsflake.lockfor dependency pinning.envrcfor direnv integration with the flake.gitignoreto exclude Nix-specific directoriesType of change
Affected areas
How to test
If you have Nix installed:
The environment will set up:
Breaking changes
Related issues
N/A
Security considerations
The Nix environment is isolated and reproducible, improving development
environment consistency.
Checklist
docs/contributing/README.mdand followed the guidelines