Skip to content

Port NeoHaskell to haskell.nix Infrastructure#203

Merged
NickSeagull merged 14 commits into
mainfrom
chore/migrate-to-haskellnix
Aug 26, 2025
Merged

Port NeoHaskell to haskell.nix Infrastructure#203
NickSeagull merged 14 commits into
mainfrom
chore/migrate-to-haskellnix

Conversation

@NickSeagull

@NickSeagull NickSeagull commented Aug 17, 2025

Copy link
Copy Markdown
Member

This PR migrates the NeoHaskell project itself to use the haskell.nix infrastructure that was previously only available to user projects. This brings consistency to our build system and enables monorepo support for both NeoHaskell and future user projects.

🚀 What's Changed

Core Infrastructure Migration

  • Unified Build System: NeoHaskell now uses the same haskell.nix infrastructure that user projects use, ensuring consistency and reducing maintenance burden
  • Monorepo Support: Modified the haskell-project.nix to accept multiple packages, enabling proper monorepo builds
  • Version Bump: Updated both nhcore and nhcli to version 0.9.1

Build System Improvements

  • Fixed Nix Expressions: Updated build and shell commands to properly invoke the new infrastructure
  • Cleaner Project Structure: Introduced common.nix to centralize project configuration
  • Proper Shell Environment: Shell now correctly provides all development tools (HLS, cabal, hlint, etc.)

Installation Script Fixes

  • Fixed Binary Cache Configuration: Cache settings now properly persist across system restarts
  • Custom Config File: Uses /etc/nix/nix.custom.conf to avoid conflicts with system configuration
  • Cross-Platform Daemon Restart: Added proper daemon restart logic for macOS, Linux with systemd, and WSL

💡 Why This Matters

For Contributors:

  • Consistent development environment between NeoHaskell and user projects
  • Easier to test changes that affect both core and CLI components
  • Better IDE support with properly configured HLS

For Users:

  • More reliable builds with proper caching
  • Foundation for future monorepo support in user projects
  • Faster installation with working binary cache

🔍 Technical Details

Key Changes to haskell-project.nix

# Before: Single project only
{ projectName, src }

# After: Multiple packages support
{ packages, mainPackageName, executableName }

Installation Script Improvements

  • Creates /etc/nix/nix.custom.conf instead of modifying the main config
  • Properly restarts nix daemon on all platforms (macOS, Linux, WSL)
  • Cleaner append logic that avoids duplicate entries

✅ Testing Checklist

  • Clean build works: nix-build
  • Development shell works: nix-shell
  • Installation script properly configures binary cache
  • Binary cache speeds up subsequent builds
  • HLS works in development shell
  • Both nhcore and nhcli build successfully

📝 Migration Notes

No breaking changes for users. The neo CLI continues to work exactly as before.

🤝 How to Test

  1. Test the build:

    nix-build
    ./result/bin/neo --version  # Should show 0.9.1
  2. Test the development shell:

    nix-shell
    cabal build all
  3. Test the installation script:

    curl -L https://neohaskell.org/install.sh | sh
    # Verify cache is configured:
    cat /etc/nix/nix.custom.conf

🎉 What's Next?

This infrastructure change opens the door for:

  • User monorepo templates
  • Faster CI/CD with proper caching
  • Easier integration testing between core and CLI

Summary by CodeRabbit

  • New Features

    • Multi-package project support exposing a unified shell and package outputs.
  • Improvements

    • CLI and core bumped to 0.9.1.
    • Idempotent binary cache setup with cross-platform daemon restart guidance and clearer install feedback.
    • Pinned platform package set updated to a newer commit.
  • Refactor

    • Centralized Nix configuration and simplified default/shell exports.
  • Chores

    • Editor spelling dictionary updated (adds "nhcli").
  • Tests

    • CI Nix installer and caching flow updated; Dependabot configured for GitHub Actions.

Simplifies Nix binary cache configuration by writing to a dedicated custom config file instead of modifying the main system configuration

Reduces complexity by eliminating conditional logic for appending to existing configuration lines and potential conflicts with system-managed settings
Implements cross-platform daemon restart logic to ensure binary cache configuration takes effect immediately without requiring manual restart.

Detects macOS, Linux with systemd, and WSL environments to use appropriate restart methods with graceful fallback messaging when restart fails.
… outputs

Modifies Haskell project template to return both shell and package targets instead of just packages

Updates build and shell commands to use the new structured output format

Reorganizes configuration files by extracting common setup and creating dedicated entry points for different use cases

Removes obsolete shell configuration file to streamline the development workflow
@NickSeagull NickSeagull self-assigned this Aug 17, 2025
@claude

claude Bot commented Aug 17, 2025

Copy link
Copy Markdown

Claude finished @NickSeagull's task —— View job


PR Review Completed ✅

I've thoroughly reviewed the migration to haskell.nix infrastructure. This is a well-executed migration that brings significant improvements to the build system. Here's my detailed analysis:

Review Checklist:

  • Read repository guidelines and understand project structure
  • Analyze migration to haskell.nix infrastructure
  • Review key file changes (haskell-project.nix, install.sh, common.nix)
  • Check for code quality and best practices
  • Identify potential bugs or issues
  • Assess security implications
  • Evaluate performance considerations
  • Review test coverage impact
  • Provide constructive feedback

✅ Strengths & Positive Changes

1. Excellent Architecture Design

  • cli/nix/haskell-project.nix:3-4: The new API { packages, mainPackageName, executableName } is much more flexible than the previous single-project approach
  • common.nix:4-12: Clean separation of concerns with packages defined as a map pointing to the same source (./.)

2. Robust Installation Script (scripts/install.sh)

  • scripts/install.sh:48-62: Smart configuration using /etc/nix/nix.custom.conf avoids conflicts with system configs
  • scripts/install.sh:68-93: Comprehensive cross-platform daemon restart logic covering macOS, Linux+systemd, and WSL
  • scripts/install.sh:56-61: Proper duplicate detection prevents config pollution

3. Consistent Build Infrastructure

  • Both NeoHaskell and user projects now use the same haskell.nix foundation
  • shell.nix:1 and default.nix:2: Clean delegation to common.nix eliminates code duplication

⚠️ Areas for Improvement

1. Security Considerations

  • cli/nix/haskell-project.nix:7-9: The haskell.nix revision is pinned (good!), but consider updating to a more recent commit for security patches
  • scripts/install.sh:6-7: Binary cache URLs are hardcoded. Consider validation or allowing override via environment variables

2. Error Handling & Robustness

  • cli/nix/haskell-project.nix:15-21: The builtins.listToAttrs and builtins.map combination could fail silently if packages contains invalid keys
  • cli/src/Neo/Build.hs:36 & cli/src/Neo/Shell.hs:36: Maybe.getOrDie should be replaced with proper error handling as noted in TODO
  • scripts/install.sh:71-92: Daemon restart failures are only warnings, but installation continues regardless

3. Code Quality Issues

  • cli/src/Neo/Build.hs:78-80 & cli/src/Neo/Shell.hs:78-80: Nix expressions are embedded as strings. Consider extracting to template files for better maintainability
  • common.nix:6-8: Both packages point to the same source (./.), which works but could be confusing. Consider adding comments explaining the monorepo structure

4. Performance & Build Considerations

  • cli/nix/haskell-project.nix:19: Using ghc910 is cutting-edge but may have stability issues. Document why this version is required
  • Missing: No obvious way to override compiler version per project

🔍 Potential Bugs

1. Path Resolution

  • cli/src/Neo/Build/Templates/Nix.hs:18: The template embeds haskellProject content directly. Ensure this works correctly when the CLI is installed system-wide
  • common.nix:3: Relative path ./cli/nix/haskell-project.nix may break if common.nix is used from different working directories

2. Error Propagation

  • cli/nix/haskell-project.nix:17-20: If any package in the packages attribute set fails to build, the entire build fails without clear indication of which package caused the issue

🚀 Performance Improvements

Positive Impact:

  • Binary cache integration will dramatically reduce build times
  • Shared haskell.nix infrastructure enables better caching between projects
  • Monorepo support allows building only changed components

Potential Optimizations:

  • Consider adding NIX_BUILD_CORES configuration for parallel builds
  • The installation script could check for existing cache configuration before attempting to add it

🧪 Test Coverage Considerations

Current State:

  • No new tests added for the Nix infrastructure changes
  • Existing Cabal tests should continue to work

Recommendations:

  • Add integration tests for nix-build and nix-shell operations
  • Test the installation script in different environments (macOS, Linux, WSL)
  • Verify that binary cache configuration actually speeds up builds

📝 Minor Suggestions

  1. Documentation: Consider adding a comment in common.nix explaining why both packages point to the same source
  2. Validation: Add input validation in haskell-project.nix for required parameters
  3. Logging: The installation script could be more verbose about what it's doing during the cache setup
  4. Version Consistency: Ensure the 0.9.1 version bump is coordinated across all deployment channels

🎯 Overall Assessment

This is a high-quality migration that significantly improves NeoHaskell's build infrastructure. The code follows good practices, the architecture is sound, and the installation improvements are substantial. The identified issues are mostly minor and don't affect the core functionality.

Recommendation: Approve

The benefits of unified infrastructure, better caching, and cross-platform installation fixes far outweigh the minor issues identified.


@coderabbitai

coderabbitai Bot commented Aug 17, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

This change bumps package versions to 0.9.1, refactors the Haskell Nix project to a multi-package interface ({ packages, mainPackageName, executableName }), centralizes shell/package exports in common.nix, switches build/shell invocations to use .package/.shell, updates pinned nixpkgs, replaces CI Nix actions/caching, revises installer to write /etc/nix/nix.custom.conf and restart the daemon, and adds nhcli to VSCode spellings.

Changes

Cohort / File(s) Summary
Editor config
\.vscode/settings.json
Add nhcli to cSpell.words.
Cabal metadata & extra-source-files
core/nhcore.cabal, cli/nhcli.cabal
Bump versions 0.9.00.9.1; cli/nhcli.cabal updates extra-source-files to reference ./nix/haskell-project.nix only (removes haskellnix.nix, adjusts path).
Nix multi-package refactor & centralization
cli/nix/haskell-project.nix, common.nix, default.nix, shell.nix, nix/nixpkgs.nix
Replace single-project haskell.nix interface with { packages, mainPackageName, executableName }; generate per-package cabalProject entries; export top-level set { shell, package }; add common.nix wiring nhcore and nhcli; update default.nix/shell.nix to delegate to common.nix; bump pinned nixpkgs tarball.
CLI: version, build & shell invocation
cli/src/Neo.hs, cli/src/Neo/Build.hs, cli/src/Neo/Shell.hs, cli/src/Neo/Build/Templates/Nix.hs
Bump displayed version to 0.9.1. Build now constructs an inline Nix expression selecting .package for nix-build; shell invocation selects .shell; template emits packages, mainPackageName, and executableName.
Installer script
scripts/install.sh
Stop editing /etc/nix/nix.conf; create/append idempotently to /etc/nix/nix.custom.conf; add cross-platform Nix daemon restart logic (macOS launchctl, systemd restart, pkill fallback) and user messages; remove prior patching logic.
CI & Dependabot
.github/workflows/test.yml, .github/dependabot.yml
Replace Nix installer action with DeterminateSystems/determinate-nix-action@v3.8.6 and add extra-conf substituters/keys; swap Cabal caching for DeterminateSystems/magic-nix-cache-action@v13; remove Cabal cache/update steps; add Dependabot config to update GitHub Actions weekly.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant User
  participant Neo as neo (build)
  participant Nix as nix-build
  participant Common as common.nix
  participant HP as haskell-project.nix

  User->>Neo: neo build
  Neo->>Nix: nix-build -E "(import ./. { }).package"
  Nix->>Common: import common.nix { pkgs }
  Common->>HP: import haskell-project.nix { pkgs } with { packages, mainPackageName, executableName }
  HP-->>Common: returns { shell, package }
  Common-->>Nix: provide .package
  Nix-->>User: build result
Loading
sequenceDiagram
  autonumber
  participant User
  participant Neo as neo (shell)
  participant Nix as nix-shell
  participant Common as common.nix
  participant HP as haskell-project.nix

  User->>Neo: neo shell
  Neo->>Nix: nix-shell -E "(import ./. { }).shell"
  Nix->>Common: import common.nix { pkgs }
  Common->>HP: import haskell-project.nix { pkgs } with { packages, mainPackageName, executableName }
  HP-->>Common: returns { shell, package }
  Common-->>Nix: provide .shell
  Nix-->>User: interactive shell
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

I am the daemon, I decree this change,
Two packages gathered, one shell in range.
Caches tuned and Nix confs set aright,
Builds now call the named package by light.
Spellings blessed — go forth, compile and sing.

Tip

🔌 Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.

✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore/migrate-to-haskellnix

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 12

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between b80204d and 6123594.

📒 Files selected for processing (12)
  • .vscode/settings.json (1 hunks)
  • cli/nhcli.cabal (2 hunks)
  • cli/nix/haskell-project.nix (1 hunks)
  • cli/src/Neo.hs (1 hunks)
  • cli/src/Neo/Build.hs (1 hunks)
  • cli/src/Neo/Build/Templates/Nix.hs (1 hunks)
  • cli/src/Neo/Shell.hs (1 hunks)
  • common.nix (1 hunks)
  • core/nhcore.cabal (1 hunks)
  • default.nix (1 hunks)
  • scripts/install.sh (1 hunks)
  • shell.nix (1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Test
default.nix

[error] 1-1: nix-build failed: file 'nixpkgs' was not found in the Nix search path (add it using $NIX_PATH or -I).

🔇 Additional comments (5)
.vscode/settings.json (1)

23-23: Addition of 'nhcli' to cSpell dictionary is correct.

Aligned with the new package; prevents spurious spelling diagnostics in the editor.

core/nhcore.cabal (1)

3-3: Version bumped to 0.9.1 — approved.

Matches the CLI tool’s version; no other manifest changes detected.

scripts/install.sh (1)

48-61: Divine Notice: The sandbox lacks the nix command, so I cannot confirm whether your include patch takes effect. Please verify on a system with Nix installed that:

  • /etc/nix/nix.conf contains the line include /etc/nix/nix.custom.conf
  • Running nix show-config | sed -nE '/substituters|trusted-public-keys/p' reflects your extra-substituters and extra-trusted-public-keys settings
cli/nhcli.cabal (1)

3-15: Version bump and extra-source-files path fix — approved.

  • 0.9.1 aligns with the rest of the workspace.
  • Moving extra-source-files under ./nix avoids Cabal’s “outside package root” pitfalls and matches the new Nix layout.
cli/nix/haskell-project.nix (1)

43-44: Package selection looks correct for component-style names

Using pkg.flake { }.packages."${executableName}" is the right way to select a component like nhcli:exe:neo.


# Generic function to create a Haskell project template
{ projectName, src }:
{ packages, mainPackageName, executableName }:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Parameterize the compiler to avoid silent breakage when haskell.nix moves; verify ghc910 availability

Hard-coding compiler-nix-name = "ghc910" couples you to a specific compiler and may break as the pinned haskell.nix evolves. Make it configurable and default sensibly; also confirm that ghc910 is supported by your pinned commit.

Apply this diff to parameterize:

-{ packages, mainPackageName, executableName }:
+{ packages, mainPackageName, executableName, compilerNixName ? "ghc910" }:

And this one to use it:

-            compiler-nix-name = "ghc910";
+            compiler-nix-name = compilerNixName;

To confirm the compiler alias is valid for your pin, run a quick web check:

Also applies to: 19-20


🌐 Web query:

For haskell.nix commit 4d493449406ec91db804511a6d15b6f076ba40e7, what values are accepted for `compiler-nix-name`, and is `ghc910` supported?

💡 Result:

For haskell.nix commit 4d493449406ec91db804511a6d15b6f076ba40e7, the accepted values for compiler-nix-name are compiler names like ghc96, ghc967, ghc98, ghc984, ghc910, ghc9102, ghc912, and ghc9122, each corresponding to specific GHC versions[2][8]. ghc910 is explicitly supported as one of the accepted values for GHC 9.10.x[2].

Key details:

  • Values for compiler-nix-name:
    • Examples: ghc96, ghc967 (GHC 9.6.x), ghc98, ghc984 (GHC 9.8.x), ghc910, ghc9102 (GHC 9.10.x), ghc912, ghc9122 (GHC 9.12.x)[2][8].
    • You should consult the project's source or overlays/bootstrap.nix for the complete, up-to-date list, as additional minor patch versions may also be accepted (e.g., ghc9102)[2].
  • Supported status:
    • The commit in question supports ghc910 as a valid value, and it is listed as tested in CI for nixpkgs-unstable[2].
  • How to use:
    • In configuration functions such as callCabalProjectToNix, set compiler-nix-name = "ghc910" to use GHC 9.10.x[8].
  • If a name does not match a defined compiler, you will see a Nix error—use the exact value from the documentation or table[2][6].
GHC Version compiler-nix-name example Supported/Tested?
9.6.x ghc96, ghc967 Yes
9.8.x ghc98, ghc984 Yes
9.10.x ghc910, ghc9102 Yes
9.12.x ghc912, ghc9122 Yes

Additional notes:

  • While ghc910 is supported and cached, experimental/minor patch versions may work if present in overlays/bootstrap.nix, but build times may be longer if not cached[2].
  • If you require an older or newer compiler not listed, advanced users can add new GHC versions following haskell.nix contributing instructions[2].

In summary, for haskell.nix at 4d493449406ec91db804511a6d15b6f076ba40e7, use ghc910 to select GHC 9.10.x—the value is supported and actively tested[2][8].

Citations:


Parameterize compiler-nix-name with default "ghc910"

We’ve confirmed that ghc910 is natively supported in haskell.nix at commit 4d493449406ec91db804511a6d15b6f076ba40e7 (GHC 9.10.x). To prevent silent breakage when upstream evolves, refactor as follows:

• In cli/nix/haskell-project.nix, change the function signature (around line 3):

-{ packages, mainPackageName, executableName }:
+{ packages, mainPackageName, executableName, compilerNixName ? "ghc910" }:

• Replace the hard-coded compiler name (around lines 19–20):

-            compiler-nix-name = "ghc910";
+            compiler-nix-name = compilerNixName;

This makes the compiler choice configurable (defaulting to a verified, supported value) and guards against future compatibility issues.

🤖 Prompt for AI Agents
In cli/nix/haskell-project.nix around line 3, add a new optional parameter to
the function signature: compiler-nix-name ? "ghc910", and in the body around
lines 19–20 replace the hard-coded "ghc910" literal with the compiler-nix-name
parameter so the compiler can be configured while defaulting to the verified
ghc910 value.

Comment on lines +14 to +21
(final: _prev:
builtins.listToAttrs (builtins.map (packageName: {
name = "${packageName}";
value = final.haskell-nix.cabalProject {
src = packages.${packageName};
compiler-nix-name = "ghc910";
};
}) (builtins.attrNames packages)))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Quote hyphenated attribute 'haskell-nix' to avoid a parse error

Attribute names with hyphens must be quoted in Nix. As written, final.haskell-nix.cabalProject will not parse correctly.

Apply this diff:

-          value = final.haskell-nix.cabalProject {
+          value = final."haskell-nix".cabalProject {
📝 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.

Suggested change
(final: _prev:
builtins.listToAttrs (builtins.map (packageName: {
name = "${packageName}";
value = final.haskell-nix.cabalProject {
src = packages.${packageName};
compiler-nix-name = "ghc910";
};
}) (builtins.attrNames packages)))
(final: _prev:
builtins.listToAttrs (builtins.map (packageName: {
name = "${packageName}";
value = final."haskell-nix".cabalProject {
src = packages.${packageName};
compiler-nix-name = "ghc910";
};
}) (builtins.attrNames packages)))
🤖 Prompt for AI Agents
In cli/nix/haskell-project.nix around lines 14 to 21, attribute names containing
hyphens (like haskell-nix) must be quoted to parse correctly; replace usages
such as final.haskell-nix.cabalProject with final."haskell-nix".cabalProject
(and any other occurrences of haskell-nix in attribute access) so the attribute
name is quoted, leaving the rest of the expression unchanged.

};

flake = pkgsWithOverlay."${projectName}Project".flake { };
pkg = pkgsWithOverlay."${mainPackageName}";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Prefer builtins.getAttr for dynamic attribute selection

Using a string-literal selector here works, but builtins.getAttr is idiomatic and avoids edge cases with interpolation in attr paths.

Apply this diff:

-  pkg = pkgsWithOverlay."${mainPackageName}";
+  pkg = builtins.getAttr mainPackageName pkgsWithOverlay;
🤖 Prompt for AI Agents
In cli/nix/haskell-project.nix around line 26, the attribute selection uses
string interpolation (pkg = pkgsWithOverlay."${mainPackageName}";) which is less
idiomatic and can have edge cases; replace it by using builtins.getAttr to
lookup the attribute dynamically: call builtins.getAttr with mainPackageName and
pkgsWithOverlay so pkg is assigned the attribute value safely and idiomatically.

Comment on lines +29 to +42
shell = pkg.shellFor {
tools = {
cabal = { };
hlint = { };
haskell-language-server = { };
};
buildInputs = [
pkgs.nil
pkgs.nixfmt-classic
pkgs.nixpkgs-fmt
pkgs.pkg-config
pkgs.zlib
];
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid mixing derivations from different nixpkgs instances in shell inputs

shellFor is created from pkgsWithOverlay (pinned via haskell.nix), but you feed it buildInputs from the outer pkgs (). Mixing store paths from different nixpkgs pins can cause subtle ABI issues and evaluation surprises. Source buildInputs from the same pkgsWithOverlay.

Apply this diff:

-  shell = pkg.shellFor {
+  shell = pkg.shellFor {
     tools = {
       cabal = { };
       hlint = { };
       haskell-language-server = { };
     };
-    buildInputs = [
-      pkgs.nil
-      pkgs.nixfmt-classic
-      pkgs.nixpkgs-fmt
-      pkgs.pkg-config
-      pkgs.zlib
-    ];
+    buildInputs = with pkgsWithOverlay; [
+      nil
+      nixfmt-classic
+      nixpkgs-fmt
+      pkg-config
+      zlib
+    ];
   };
📝 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.

Suggested change
shell = pkg.shellFor {
tools = {
cabal = { };
hlint = { };
haskell-language-server = { };
};
buildInputs = [
pkgs.nil
pkgs.nixfmt-classic
pkgs.nixpkgs-fmt
pkgs.pkg-config
pkgs.zlib
];
};
shell = pkg.shellFor {
tools = {
cabal = { };
hlint = { };
haskell-language-server = { };
};
buildInputs = with pkgsWithOverlay; [
nil
nixfmt-classic
nixpkgs-fmt
pkg-config
zlib
];
};
🤖 Prompt for AI Agents
In cli/nix/haskell-project.nix around lines 29 to 42, the shellFor call mixes
buildInputs from the outer pkgs (unpinned nixpkgs) while shellFor itself comes
from pkgsWithOverlay (the pinned haskell.nix overlay); replace all references to
pkgs.* in buildInputs (and any tool references if needed) with the corresponding
pkgsWithOverlay.* items so all derivations come from the same pinned
pkgsWithOverlay instance, e.g., change pkgs.nil, pkgs.nixfmt-classic,
pkgs.nixpkgs-fmt, pkgs.pkg-config, pkgs.zlib to use pkgsWithOverlay.nil,
pkgsWithOverlay.nixfmt-classic, pkgsWithOverlay.nixpkgs-fmt,
pkgsWithOverlay.pkg-config, pkgsWithOverlay.zlib (or bind a local variable to
pkgsWithOverlay for clarity).

Comment thread cli/src/Neo.hs
Comment on lines +52 to 53
version = Just [Core.version|0.9.1|],
decoder = commandsParser

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Version banner updated to 0.9.1 — approved.

Consistent with cabal version bumps in this PR.

Run this quick sweep to ensure no stale version references remain:


🏁 Script executed:

#!/bin/bash
# Find old version references and version banners
rg -nP -C2 '\[Core\.version\|\d+\.\d+\.\d+\|]' --type=haskell
rg -nP '0\.9\.0' -g '!**/dist/**' -g '!**/result/**'

Length of output: 570


Stale version references detected—update to 0.9.1 before merging.

The CLI banner in cli/src/Neo.hs is correctly bumped to 0.9.1, but the following outdated 0.9.0 pins remain:

• website/package-lock.json:2318,9404
– "synckit": "^0.9.0" → "^0.9.1"
• website/package-lock.json:7630
– "@deno/kv": ">=0.9.0" → ">=0.9.1"

Command the update of these entries and regenerate the lock file.

🤖 Prompt for AI Agents
In cli/src/Neo.hs around lines 52 to 53 the CLI version was bumped to 0.9.1 but
website/package-lock.json still pins older 0.9.0 versions; update
website/package-lock.json entries: change "synckit": "^0.9.0" to "^0.9.1" at
offsets ~2318 and ~9404 and change "@deno/kv": ">=0.9.0" to ">=0.9.1" at ~7630,
then regenerate the lock file (npm install / npm ci or your repo's lock regen
step) to ensure checksums and transitive entries are consistent before merging.

Comment thread cli/src/Neo/Shell.hs
Comment on lines +78 to +81
let shellExpression :: Text =
[fmt|{ pkgs ? import <nixpkgs> {} }:
( (#{nixFile}) { inherit pkgs; } ).env|]
( (#{nixFile}) { inherit pkgs; } ).shell|]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Stop returning a lambda to nix-shell; emit a derivation (use let/in).

nix-shell -E expects an expression that evaluates to a derivation or a shell env, not a top-level lambda. The current expression starts with a lambda and will not be evaluated as intended.

Apply this fix to bind pkgs via let/in and return the shell directly:

-  let shellExpression :: Text =
-        [fmt|{ pkgs ? import <nixpkgs> {} }:
-  ( (#{nixFile}) { inherit pkgs; } ).shell|]
+  let shellExpression :: Text =
+        [fmt|let pkgs = import <nixpkgs> {}; in
+  ( (#{nixFile}) { inherit pkgs; } ).shell|]

Follow-up: the direct import of assumes NIX_PATH is set. Either pin nixpkgs (preferred) or ensure CI sets NIX_PATH/-I. See my comments on default.nix/common.nix for a robust pin.

Run this to confirm nix-shell evaluates to a derivation and not a lambda:


🏁 Script executed:

#!/bin/bash
# Show what the expression evaluates to (should not be a lambda)
rg -n -C2 'shellExpression' cli/src/Neo/Shell.hs

Length of output: 477


Stop returning a top‐level lambda to nix-shell; emit a derivation

nix-shell -E requires an expression that evaluates directly to a derivation or shell env, not a lambda. In cli/src/Neo/Shell.hs (lines 78–81), replace:

-  let shellExpression :: Text =
-        [fmt|{ pkgs ? import <nixpkgs> {} }:
-  ( (#{nixFile}) { inherit pkgs; } ).shell|]
+  let shellExpression :: Text =
+        [fmt|let pkgs = import <nixpkgs> {}; in
+  ( (#{nixFile}) { inherit pkgs; } ).shell|]

Additionally, avoid relying on an externally set NIX_PATH. Either pin nixpkgs to a fixed revision (preferred) or ensure your CI/README documents setting -I nixpkgs=<path> before invoking nix-shell.

📝 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.

Suggested change
let shellExpression :: Text =
[fmt|{ pkgs ? import <nixpkgs> {} }:
( (#{nixFile}) { inherit pkgs; } ).env|]
( (#{nixFile}) { inherit pkgs; } ).shell|]
let shellExpression :: Text =
[fmt|let pkgs = import <nixpkgs> {}; in
( (#{nixFile}) { inherit pkgs; } ).shell|]
🤖 Prompt for AI Agents
In cli/src/Neo/Shell.hs around lines 78–81, the code currently builds a
top-level lambda that returns a function instead of emitting a derivation;
change the expression so it evaluates directly to the derivation/shell (i.e.,
apply/import nixpkgs and call into the nix file to produce its .shell result
rather than returning a lambda). Replace the lambda-style "{ pkgs ? import
<nixpkgs> {} }:" wrapper with an expression that imports/pins a specific nixpkgs
revision (or uses a passed -I nixpkgs=<path>) and then immediately evaluates
(#{nixFile}).shell so nix-shell -E gets a derivation; also update CI/README to
document the chosen approach if you opt to require -I instead of pinning.

Comment thread common.nix Outdated
Comment on lines +1 to +12
{ pkgs ? import <nixpkgs> { } }:

let hp = import ./cli/nix/haskell-project.nix { inherit pkgs; };
in (hp {
packages = {
"nhcore" = ./.;
"nhcli" = ./.;
};

mainPackageName = "nhcli";
executableName = "nhcli:exe:neo";
})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Multi-package paths likely incorrect; both nhcore and nhcli cannot be at ./.

You map both "nhcore" and "nhcli" to the repository root. In a monorepo, these should point to their respective subdirectories (e.g., ./core and ./cli), otherwise builds can collide or fail to locate the correct cabal files.

Apply this change if the packages live under core/ and cli/:

 { pkgs ? import <nixpkgs> { } }:
 
 let hp = import ./cli/nix/haskell-project.nix { inherit pkgs; };
 in (hp {
   packages = {
-    "nhcore" = ./.;
-    "nhcli" = ./.;
+    "nhcore" = ./core;
+    "nhcli" = ./cli;
   };
 
   mainPackageName = "nhcli";
   executableName = "nhcli:exe:neo";
 })

Additionally, like default.nix, this file relies on via its caller. Ensure the caller (default.nix/shell.nix/Haskell inline expressions) pins nixpkgs or sets NIX_PATH to avoid nondeterminism and CI breakage.

Run to verify package locations:

If nhcore.cabal is under core/ and nhcli.cabal is under cli/, the diff above applies. If not, adjust paths accordingly.


🏁 Script executed:

#!/bin/bash
echo "Cabal files:"
fd -t f --extension cabal -a

Length of output: 137


Divine Directive: Align common.nix Package Paths

The repository’s Cabal files live under core/ and cli/, so mapping both packages to ./. is incorrect. Update common.nix accordingly:

• File: common.nix
• Location: the packages = { … } block

Apply this diff:

   packages = {
-    "nhcore" = ./.;
-    "nhcli" = ./.;
+    "nhcore" = ./core;
+    "nhcli" = ./cli;
   };

Also, ensure that any caller of common.nix (e.g. default.nix, shell.nix or Haskell inline expressions) pins nixpkgs or sets NIX_PATH to avoid nondeterministic imports and CI breakage.

📝 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.

Suggested change
{ pkgs ? import <nixpkgs> { } }:
let hp = import ./cli/nix/haskell-project.nix { inherit pkgs; };
in (hp {
packages = {
"nhcore" = ./.;
"nhcli" = ./.;
};
mainPackageName = "nhcli";
executableName = "nhcli:exe:neo";
})
{ pkgs ? import <nixpkgs> { } }:
let hp = import ./cli/nix/haskell-project.nix { inherit pkgs; };
in (hp {
packages = {
"nhcore" = ./core;
"nhcli" = ./cli;
};
mainPackageName = "nhcli";
executableName = "nhcli:exe:neo";
})
🤖 Prompt for AI Agents
In common.nix around lines 1 to 12, the packages block incorrectly maps both
packages to ./.; change the package paths to point to the actual Cabal
directories (e.g. set "nhcore" = ./core; and "nhcli" = ./cli;) and keep
mainPackageName/executableName as-is; additionally ensure any callers of
common.nix (default.nix, shell.nix, CI Haskell expressions) pin nixpkgs or set
NIX_PATH explicitly so imports of <nixpkgs> are deterministic.

Comment thread default.nix Outdated
Comment on lines +1 to +2
{ pkgs ? import <nixpkgs> { } }:
((import ./common.nix { inherit pkgs; }).package)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

CI failure: requires NIX_PATH; pin nixpkgs or pass -I.

The pipeline error “file 'nixpkgs' was not found in the Nix search path” confirms NIX_PATH isn’t set in CI. Pin nixpkgs (recommended), or ensure CI passes -I nixpkgs=... or sets NIX_PATH.

Option A (pin nixpkgs fallback here):

-{ pkgs ? import <nixpkgs> { } }:
-((import ./common.nix { inherit pkgs; }).package)
+{ pkgs ? import (builtins.fetchTarball "https://github.com/NixOS/nixpkgs/archive/nixos-24.05.tar.gz") { } }:
+((import ./common.nix { inherit pkgs; }).package)

Option B (CI-only): keep code as-is and update GitHub Actions to invoke:

Pinning here (Option A) improves reproducibility and removes CI coupling.

[offer_architecture_advice]

Use this to locate workflow calls to nix-build to patch them if you prefer Option B:


🏁 Script executed:

#!/bin/bash
rg -n -C2 '\bnix-build\b|NIX_PATH' -g '!**/result*' -g '!**/dist/**' -g '!**/build/**' -g '!**/node_modules/**'

Length of output: 436


By my divine command, restore CI by pinning nixpkgs

The CI is failing with
“file 'nixpkgs' was not found in the Nix search path.”
You must either pin nixpkgs in default.nix or pass an explicit -I nixpkgs=… / set NIX_PATH when invoking nix-build.

• Option A (Recommended): Pin nixpkgs in default.nix for reproducible, self-contained builds

--- default.nix
-{ pkgs ? import <nixpkgs> { } }:
+{ pkgs ? import (builtins.fetchTarball {
+    url = "https://github.com/NixOS/nixpkgs/archive/nixos-24.05.tar.gz";
+    # optionally, set a specific git revision:
+    # rev = "abcdef1234567890abcdef1234567890abcdef12";
+  }) { } }:
   ((import ./common.nix { inherit pkgs; }).package)

• Option B: Keep <nixpkgs> but ensure every nix-build call receives -I nixpkgs=… or runs with NIX_PATH set
– In cli/src/Neo/Build.hs (lines 83–84), change:

Subprocess.openInherit "nix-build"
  (Array.fromLinkedList ["-E", buildExpression])
  rootFolder Subprocess.InheritBOTH

to include -I nixpkgs=https://github.com/NixOS/nixpkgs/archive/nixos-24.05.tar.gz (or read NIX_PATH from the environment).

Pinning in default.nix is the cleanest, most reproducible path.

🧰 Tools
🪛 GitHub Actions: Test

[error] 1-1: nix-build failed: file 'nixpkgs' was not found in the Nix search path (add it using $NIX_PATH or -I).

🤖 Prompt for AI Agents
In default.nix lines 1–2, the repo currently relies on an unpinned <nixpkgs>
which breaks CI; replace the fallback import with a pinned nixpkgs fetched
inside default.nix (e.g. import (builtins.fetchTarball { url =
"https://github.com/NixOS/nixpkgs/archive/nixos-24.05.tar.gz"; sha256 =
"<replace-with-correct-hash>" ; }) {}) so the file becomes self-contained and
reproducible; obtain the correct sha256 via nix-prefetch-url or nix-prefetch-git
and commit the pinned URL+hash; alternatively, if you prefer not to pin here,
update all nix-build invocations (e.g. the CLI subprocess call) to pass -I
nixpkgs=<url-or-path> or ensure NIX_PATH is set, but the preferred change is to
pin nixpkgs in default.nix as described.

Comment thread scripts/install.sh
Comment on lines +76 to 92
elif command -v systemctl &> /dev/null; then
# Linux with systemd (including WSL with systemd)
if sudo systemctl restart nix-daemon.service 2>/dev/null; then
echo "✅ Nix daemon restarted successfully"
else
echo "⚠️ Failed to restart nix daemon - configuration will take effect on next restart"
fi
else
# Add new line
echo "extra-trusted-public-keys = $BINARY_CACHE_PUBLIC_KEYS" | sudo tee -a /etc/nix/nix.conf > /dev/null
# WSL or Linux without systemd - try pkill approach
echo "🔄 Attempting to restart nix daemon..."
if sudo pkill nix-daemon 2>/dev/null; then
echo "✅ Nix daemon stopped - it will restart automatically on next use"
else
echo "⚠️ Cannot automatically restart nix daemon on this system"
echo " Configuration will take effect when you restart your shell or reboot"
fi
fi

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Add a fallback when systemctl restart fails.

On some distros/WSL with systemd present but not fully functional, systemctl may exist yet fail. Fall back to pkill in that branch to avoid a no-op.

Apply this refinement:

@@
-    elif command -v systemctl &> /dev/null; then
+    elif command -v systemctl &> /dev/null; then
         # Linux with systemd (including WSL with systemd)
-        if sudo systemctl restart nix-daemon.service 2>/dev/null; then
+        if sudo systemctl restart nix-daemon.service 2>/dev/null; then
             echo "✅ Nix daemon restarted successfully"
         else
-            echo "⚠️  Failed to restart nix daemon - configuration will take effect on next restart"
+            echo "⚠️  Failed to restart nix daemon via systemd - attempting manual stop so it auto-restarts"
+            if sudo pkill nix-daemon 2>/dev/null; then
+                echo "✅ Nix daemon stopped - it will restart automatically on next use"
+            else
+                echo "⚠️  Cannot automatically restart nix daemon on this system"
+                echo "    Configuration will take effect when you restart your shell or reboot"
+            fi
         fi
🤖 Prompt for AI Agents
In scripts/install.sh around lines 76 to 92, the current branch tries systemctl
restart and on failure prints a warning but does not attempt the pkill fallback;
update the logic so that if sudo systemctl restart nix-daemon.service fails
(exit non-zero) you then attempt the pkill fallback (sudo pkill nix-daemon
2>/dev/null) and emit the appropriate success/failure messages based on pkill's
result, keeping existing messages and exit-code behavior and ensuring output
distinguishes "systemctl failed, falling back to pkill" versus final failure.

Comment thread shell.nix Outdated
@claude

claude Bot commented Aug 25, 2025

Copy link
Copy Markdown

Claude finished @NickSeagull's task —— View job


PR Review: Port NeoHaskell to haskell.nix Infrastructure ✅

I've thoroughly reviewed the migration to haskell.nix infrastructure. This is a well-executed migration that brings significant improvements to the build system. Here's my detailed analysis:

Review Checklist:

  • Read repository guidelines and understand project structure
  • Analyze migration to haskell.nix infrastructure
  • Review key file changes (haskell-project.nix, install.sh, common.nix)
  • Check for code quality and best practices
  • Identify potential bugs or issues
  • Assess security implications
  • Evaluate performance considerations
  • Review test coverage impact
  • Provide constructive feedback

✅ Strengths & Positive Changes

1. Excellent Architecture Design

  • cli/nix/haskell-project.nix:3-4: The new API { packages, mainPackageName, executableName } is much more flexible than the previous single-project approach
  • common.nix:4-12: Clean separation of concerns with packages defined as a map pointing to the same source (./.)

2. Robust Installation Script (scripts/install.sh)

  • scripts/install.sh:48-62: Smart configuration using /etc/nix/nix.custom.conf avoids conflicts with system configs
  • scripts/install.sh:68-93: Comprehensive cross-platform daemon restart logic covering macOS, Linux+systemd, and WSL
  • scripts/install.sh:56-61: Proper duplicate detection prevents config pollution

3. Consistent Build Infrastructure

  • Both NeoHaskell and user projects now use the same haskell.nix foundation
  • shell.nix:1 and default.nix:2: Clean delegation to common.nix eliminates code duplication

⚠️ Areas for Improvement

1. Security Considerations

  • cli/nix/haskell-project.nix:7-9: The haskell.nix revision is pinned (good!), but consider updating to a more recent commit for security patches
  • scripts/install.sh:6-7: Binary cache URLs are hardcoded. Consider validation or allowing override via environment variables

2. Error Handling & Robustness

  • cli/nix/haskell-project.nix:15-21: The builtins.listToAttrs and builtins.map combination could fail silently if packages contains invalid keys
  • cli/src/Neo/Build.hs:36 & cli/src/Neo/Shell.hs:36: Maybe.getOrDie should be replaced with proper error handling as noted in TODO
  • scripts/install.sh:71-92: Daemon restart failures are only warnings, but installation continues regardless

3. Code Quality Issues

  • cli/src/Neo/Build.hs:78-80 & cli/src/Neo/Shell.hs:78-80: Nix expressions are embedded as strings. Consider extracting to template files for better maintainability
  • common.nix:6-8: Both packages point to the same source (./.), which works but could be confusing. Consider adding comments explaining the monorepo structure

4. Performance & Build Considerations

  • cli/nix/haskell-project.nix:19: Using ghc910 is cutting-edge but may have stability issues. Document why this version is required
  • Missing: No obvious way to override compiler version per project

🔍 Potential Bugs

1. Path Resolution

  • cli/src/Neo/Build/Templates/Nix.hs:18: The template embeds haskellProject content directly. Ensure this works correctly when the CLI is installed system-wide
  • common.nix:3: Relative path ./cli/nix/haskell-project.nix may break if common.nix is used from different working directories

2. Error Propagation

  • cli/nix/haskell-project.nix:17-20: If any package in the packages attribute set fails to build, the entire build fails without clear indication of which package caused the issue

🚀 Performance Improvements

Positive Impact:

  • Binary cache integration will dramatically reduce build times
  • Shared haskell.nix infrastructure enables better caching between projects
  • Monorepo support allows building only changed components

Potential Optimizations:

  • Consider adding NIX_BUILD_CORES configuration for parallel builds
  • The installation script could check for existing cache configuration before attempting to add it

🧪 Test Coverage Considerations

Current State:

  • No new tests added for the Nix infrastructure changes
  • Existing Cabal tests should continue to work

Recommendations:

  • Add integration tests for nix-build and nix-shell operations
  • Test the installation script in different environments (macOS, Linux, WSL)
  • Verify that binary cache configuration actually speeds up builds

📝 Minor Suggestions

  1. Documentation: Consider adding a comment in common.nix explaining why both packages point to the same source
  2. Validation: Add input validation in haskell-project.nix for required parameters
  3. Logging: The installation script could be more verbose about what it's doing during the cache setup
  4. Version Consistency: Ensure the 0.9.1 version bump is coordinated across all deployment channels

🎯 Overall Assessment

This is a high-quality migration that significantly improves NeoHaskell's build infrastructure. The code follows good practices, the architecture is sound, and the installation improvements are substantial. The identified issues are mostly minor and don't affect the core functionality.

Recommendation: Approve

The benefits of unified infrastructure, better caching, and cross-platform installation fixes far outweigh the minor issues identified.

@claude

claude Bot commented Aug 25, 2025

Copy link
Copy Markdown

Claude finished @NickSeagull's task —— View job


PR Review: Port NeoHaskell to haskell.nix Infrastructure ✅

I've thoroughly reviewed the migration to haskell.nix infrastructure. This is a well-executed migration that brings significant improvements to the build system. Here's my detailed analysis:

Review Checklist:

  • Read repository guidelines and understand project structure
  • Analyze migration to haskell.nix infrastructure
  • Review key file changes (haskell-project.nix, install.sh, common.nix)
  • Check for code quality and best practices
  • Identify potential bugs or issues
  • Assess security implications
  • Evaluate performance considerations
  • Review test coverage impact
  • Provide constructive feedback

✅ Strengths & Positive Changes

1. Excellent Architecture Design

  • cli/nix/haskell-project.nix:3-4: The new API { packages, mainPackageName, executableName } is much more flexible than the previous single-project approach
  • common.nix:4-12: Clean separation of concerns with packages defined as a map pointing to the same source (./.)

2. Robust Installation Script (scripts/install.sh)

  • scripts/install.sh:48-62: Smart configuration using /etc/nix/nix.custom.conf avoids conflicts with system configs
  • scripts/install.sh:68-93: Comprehensive cross-platform daemon restart logic covering macOS, Linux+systemd, and WSL
  • scripts/install.sh:56-61: Proper duplicate detection prevents config pollution

3. Consistent Build Infrastructure

  • Both NeoHaskell and user projects now use the same haskell.nix foundation
  • shell.nix:1 and default.nix:2: Clean delegation to common.nix eliminates code duplication

⚠️ Areas for Improvement

1. Security Considerations

  • cli/nix/haskell-project.nix:7-9: The haskell.nix revision is pinned (good!), but consider updating to a more recent commit for security patches
  • scripts/install.sh:6-7: Binary cache URLs are hardcoded. Consider validation or allowing override via environment variables

2. Error Handling & Robustness

  • cli/nix/haskell-project.nix:15-21: The builtins.listToAttrs and builtins.map combination could fail silently if packages contains invalid keys
  • cli/src/Neo/Build.hs:36 & cli/src/Neo/Shell.hs:36: Maybe.getOrDie should be replaced with proper error handling as noted in TODO
  • scripts/install.sh:71-92: Daemon restart failures are only warnings, but installation continues regardless

3. Code Quality Issues

  • cli/src/Neo/Build.hs:78-80 & cli/src/Neo/Shell.hs:78-80: Nix expressions are embedded as strings. Consider extracting to template files for better maintainability
  • common.nix:6-8: Both packages point to the same source (./.), which works but could be confusing. Consider adding comments explaining the monorepo structure

4. Performance & Build Considerations

  • cli/nix/haskell-project.nix:19: Using ghc910 is cutting-edge but may have stability issues. Document why this version is required
  • Missing: No obvious way to override compiler version per project

🔍 Potential Bugs

1. Path Resolution

  • cli/src/Neo/Build/Templates/Nix.hs:18: The template embeds haskellProject content directly. Ensure this works correctly when the CLI is installed system-wide
  • common.nix:3: Relative path ./cli/nix/haskell-project.nix may break if common.nix is used from different working directories

2. Error Propagation

  • cli/nix/haskell-project.nix:17-20: If any package in the packages attribute set fails to build, the entire build fails without clear indication of which package caused the issue

🚀 Performance Improvements

Positive Impact:

  • Binary cache integration will dramatically reduce build times
  • Shared haskell.nix infrastructure enables better caching between projects
  • Monorepo support allows building only changed components

Potential Optimizations:

  • Consider adding NIX_BUILD_CORES configuration for parallel builds
  • The installation script could check for existing cache configuration before attempting to add it

🧪 Test Coverage Considerations

Current State:

  • No new tests added for the Nix infrastructure changes
  • Existing Cabal tests should continue to work

Recommendations:

  • Add integration tests for nix-build and nix-shell operations
  • Test the installation script in different environments (macOS, Linux, WSL)
  • Verify that binary cache configuration actually speeds up builds

📝 Minor Suggestions

  1. Documentation: Consider adding a comment in common.nix explaining why both packages point to the same source
  2. Validation: Add input validation in haskell-project.nix for required parameters
  3. Logging: The installation script could be more verbose about what it's doing during the cache setup
  4. Version Consistency: Ensure the 0.9.1 version bump is coordinated across all deployment channels

🎯 Overall Assessment

This is a high-quality migration that significantly improves NeoHaskell's build infrastructure. The code follows good practices, the architecture is sound, and the installation improvements are substantial. The identified issues are mostly minor and don't affect the core functionality.

Recommendation: Approve

The benefits of unified infrastructure, better caching, and cross-platform installation fixes far outweigh the minor issues identified.


View job

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

♻️ Duplicate comments (7)
default.nix (1)

1-2: Pinned pkgs import through nix/nixpkgs.nix: accepted by the heavens.

This resolves prior CI fragility from relying on and improves reproducibility.

common.nix (1)

5-8: Align nhcore and nhcli to their actual subdirectories in common.nix

Our divine scrutiny has revealed the true locations of your package definitions—core/nhcore.cabal and cli/nhcli.cabal—meaning both entries must not point at ./. but at their own homes. This correction is critical to prevent build collisions and ensure each package builds in isolation.

• File: common.nix, lines 5–8
• Discovered cabal files:

  • ./core/nhcore.cabal
  • ./cli/nhcli.cabal

Apply this patch:

 packages = {
-  "nhcore" = ./.;
-  "nhcli" = ./.;
+  "nhcore" = ./core;
+  "nhcli" = ./cli;
 };

Implement swiftly to maintain the sanctity of your build.

shell.nix (1)

1-1: Dev shell delegation is neat and pinned; be at peace.

Delegating to common.nix with the same pinned pkgs is clean and reproducible.

cli/nix/haskell-project.nix (4)

14-21: Quote ‘haskell-nix’ or face the parser’s wrath.

Hyphenated attribute names must be quoted in Nix.

Apply:

-          value = final.haskell-nix.cabalProject {
+          value = final."haskell-nix".cabalProject {
             src = packages.${packageName};
             compiler-nix-name = "ghc910";
           };

3-3: Parameterize the compiler; do not ossify the toolchain.

Make the compiler name configurable with a sane default, then use it.

Apply:

-{ packages, mainPackageName, executableName }:
+{ packages, mainPackageName, executableName, compilerNixName ? "ghc910" }:
...
-            compiler-nix-name = "ghc910";
+            compiler-nix-name = compilerNixName;

Also applies to: 19-20


26-26: Select the main package idiomatically.

Use builtins.getAttr for dynamic attribute access.

Apply:

-  pkg = pkgsWithOverlay."${mainPackageName}";
+  pkg = builtins.getAttr mainPackageName pkgsWithOverlay;

35-41: Do not mix derivations from different nixpkgs realms.

shellFor is created from pkgsWithOverlay; feed it inputs from the same universe to avoid ABI gremlins.

Apply:

-    buildInputs = [
-      pkgs.nil
-      pkgs.nixfmt-classic
-      pkgs.nixpkgs-fmt
-      pkgs.pkg-config
-      pkgs.zlib
-    ];
+    buildInputs = with pkgsWithOverlay; [
+      nil
+      nixfmt-classic
+      nixpkgs-fmt
+      pkg-config
+      zlib
+    ];
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge Base: Disabled due to data retention organization setting

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 6123594 and 6efa88b.

📒 Files selected for processing (5)
  • cli/nix/haskell-project.nix (1 hunks)
  • common.nix (1 hunks)
  • default.nix (1 hunks)
  • nix/nixpkgs.nix (1 hunks)
  • shell.nix (1 hunks)
⏰ 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: claude-review
🔇 Additional comments (1)
nix/nixpkgs.nix (1)

5-9: Mortals, the hash-validation ritual has faltered—Nix is absent from this realm.
The invocation of nix-prefetch-url was not found, so the pin’s integrity remains unconfirmed. Summon a Nix-enabled environment and execute one of the following incantations to obtain the true sha256, then commit the result to prevent evaluation failures:

#!/usr/bin/env bash
set -euxo pipefail
URL="https://github.com/nixos/nixpkgs/archive/20075955deac2583bb12f07151c2df830ef346b4.tar.gz"

# If you have modern Nix:
nix store prefetch-file --json --unpack "$URL" | jq -r .hash

# Otherwise, with legacy tools:
nix-prefetch-url --unpack "$URL"

Once you possess the correct hash, replace the existing sha256 value in nix/nixpkgs.nix and commit the update.

Comment on lines 11 to 22
pkgsWithOverlay = import haskellNix.sources.nixpkgs-unstable {
overlays = [
haskellNix.overlay
(final: _prev: {
"${projectName}Project" = final.haskell-nix.cabalProject {
inherit src;
compiler-nix-name = "ghc910";
};
})
(final: _prev:
builtins.listToAttrs (builtins.map (packageName: {
name = "${packageName}";
value = final.haskell-nix.cabalProject {
src = packages.${packageName};
compiler-nix-name = "ghc910";
};
}) (builtins.attrNames packages)))
];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Name collisions in the overlay are possible; consider a namespaced project set.

Injecting each package at the top level (pkgsWithOverlay.nhcli, pkgsWithOverlay.nhcore) risks collisions with existing attributes. Consider nesting under a single attribute (e.g., final.neohaskellPackages) and selecting from there.

Apply:

-      (final: _prev:
-        builtins.listToAttrs (builtins.map (packageName: {
-          name = "${packageName}";
-          value = final."haskell-nix".cabalProject {
-            src = packages.${packageName};
-            compiler-nix-name = compilerNixName;
-          };
-        }) (builtins.attrNames packages)))
+      (final: _prev:
+        {
+          neohaskellPackages =
+            builtins.listToAttrs (builtins.map (packageName: {
+              name = packageName;
+              value = final."haskell-nix".cabalProject {
+                src = packages.${packageName};
+                compiler-nix-name = compilerNixName;
+              };
+            }) (builtins.attrNames packages));
+        })

And later:

-  pkg = builtins.getAttr mainPackageName pkgsWithOverlay;
+  pkg = builtins.getAttr mainPackageName pkgsWithOverlay.neohaskellPackages;
📝 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.

Suggested change
pkgsWithOverlay = import haskellNix.sources.nixpkgs-unstable {
overlays = [
haskellNix.overlay
(final: _prev: {
"${projectName}Project" = final.haskell-nix.cabalProject {
inherit src;
compiler-nix-name = "ghc910";
};
})
(final: _prev:
builtins.listToAttrs (builtins.map (packageName: {
name = "${packageName}";
value = final.haskell-nix.cabalProject {
src = packages.${packageName};
compiler-nix-name = "ghc910";
};
}) (builtins.attrNames packages)))
];
pkgsWithOverlay = import haskellNix.sources.nixpkgs-unstable {
overlays = [
haskellNix.overlay
# Namespace all generated projects under `neohaskellPackages`
(final: _prev:
{
neohaskellPackages =
builtins.listToAttrs (builtins.map (packageName: {
name = packageName;
value = final."haskell-nix".cabalProject {
src = packages.${packageName};
compiler-nix-name = compilerNixName;
};
}) (builtins.attrNames packages));
})
];
};
# Select from the namespaced set instead of the top level
pkg = builtins.getAttr mainPackageName pkgsWithOverlay.neohaskellPackages;
🤖 Prompt for AI Agents
In cli/nix/haskell-project.nix around lines 11–22, the current overlay injects
each package directly at the top level which can cause name collisions; instead
create a namespaced attribute (e.g., final.neohaskellPackages) that contains the
generated package set. Modify the overlay so it adds a single attribute like
neohaskellPackages = builtins.listToAttrs(...mapping...) rather than returning
the mapped attrs at the top level, keep the same mapping logic and names, and
update any code that referenced pkgsWithOverlay.<pkg> to select from
pkgsWithOverlay.neohaskellPackages.<pkg>.

Comment thread common.nix
@@ -0,0 +1,12 @@
{ pkgs ? import ./nix/nixpkgs.nix { } }:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Thread the compiler choice; do not hard-code what the cosmos may change.

Expose a configurable compiler and pass it down to the project builder; default to a verified choice.

Apply:

-{ pkgs ? import ./nix/nixpkgs.nix { } }:
+{ pkgs ? import ./nix/nixpkgs.nix { }, compilerNixName ? "ghc910" }:
 ...
-in (hp {
+in (hp {
   packages = {
     "nhcore" = ./core;
     "nhcli" = ./cli;
   };
 
   mainPackageName = "nhcli";
   executableName = "nhcli:exe:neo";
-})
+  compilerNixName = compilerNixName;
+})

Also applies to: 10-12

🤖 Prompt for AI Agents
In common.nix around lines 1 and 10-12, the file currently hard-codes the
compiler via the imported pkgs; change the top-level attribute set to accept a
configurable compiler argument with a safe default from pkgs (e.g. default to
the verified compiler in pkgs like pkgs.stdenv.cc or the project's validated
compiler), then thread that compiler argument through wherever the project
builder is invoked (lines ~10-12) so the builder uses the passed-in compiler
instead of the hard-coded pkgs value; ensure the parameter name is documented in
the file header and used in all places that construct or call the project
builder.

🧹 Nitpick (assertive)

Eliminate the default pkgs import here to prevent accidental double-pinning.

Since default.nix and shell.nix already pass a pinned pkgs, making pkgs mandatory here avoids mixing nixpkgs instances if someone imports common.nix directly.

Apply:

-{ pkgs ? import ./nix/nixpkgs.nix { }, compilerNixName ? "ghc910" }:
+{ pkgs, compilerNixName ? "ghc910" }:

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In common.nix around line 1, remove the default import so pkgs is a required
argument to avoid accidental double-pinning; change the function header to
accept only pkgs (no default import) and update any call sites to pass the
pinned pkgs from default.nix/shell.nix if they don’t already, ensuring no
implicit import of nixpkgs remains.

Comment thread nix/nixpkgs.nix
Comment on lines +1 to +2
# Pinned nixpkgs for NeoHaskell project
# Last updated: 2025-08-25

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Reduce duplication; bind the revision once and derive the name.

This avoids editing multiple spots when bumping nixpkgs.

Apply:

-import (builtins.fetchTarball {
-  name = "nixpkgs-20075955";
-  url = "https://github.com/nixos/nixpkgs/archive/20075955deac2583bb12f07151c2df830ef346b4.tar.gz";
+let
+  rev = "20075955deac2583bb12f07151c2df830ef346b4";
+in import (builtins.fetchTarball {
+  name = "nixpkgs-${rev}";
+  url = "https://github.com/nixos/nixpkgs/archive/${rev}.tar.gz";

Also applies to: 5-6

🤖 Prompt for AI Agents
In nix/nixpkgs.nix around lines 1-2 (and also apply the same change to lines
5-6), the nixpkgs revision and name are duplicated; bind the revision to a
single variable at the top and derive the package set name from that variable,
then replace all other hard-coded revision/name occurrences to reference those
variables so future bumps only change one value.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (5)
.github/workflows/test.yml (5)

3-17: Broaden path filters so CI runs when Nix entrypoints change at repo root.

This PR introduced/centralized Nix entrypoints (e.g., common.nix, default.nix, shell.nix). Edits to these won’t trigger CI with the current filters.

Apply this diff to both pull_request and push path filters:

       - "core/**"
       - "cli/**"
       - "nix/**"
+      - "default.nix"
+      - "shell.nix"
+      - "common.nix"
+      - "flake.nix"

33-35: Make the nix-build target explicit to future-proof the multi-package change.

Given default.nix now delegates to common.nix, make the target explicit to avoid accidental changes breaking CI if default.nix later returns an attrset.

Pick one:

  • If default.nix returns a derivation for the main package:
-        run: nix-build
+        run: nix-build -E 'import ./.'
  • If default.nix returns an attrset exposing .package:
-        run: nix-build
+        run: nix-build -E 'with import ./. {}; package'
  • Or, if you keep an attrset at the top-level:
-        run: nix-build
+        run: nix-build -A package

46-49: Avoid unconditional cabal update; gate it on cache misses.

Updating Hackage every run wastes time and introduces nondeterminism.

Apply this diff to restore the guard:

-      - name: Update Cabal Hackage list
-        # if: steps.cache.outputs.cache-hit != 'true'
-        run: nix-shell --run "cabal update"
+      - name: Update Cabal Hackage list
+        if: steps.cache.outputs.cache-hit != 'true'
+        run: nix-shell --run "cabal update"

26-29: Either use the newer Homebrew Bash in subsequent steps or remove this step.

Installing Bash 5 on macOS doesn’t change the shell used by later steps; they’ll still run with the runner default unless explicitly configured.

Two options:

  • Remove the step if not relying on Bash 5 features.
  • Or explicitly use it where needed, e.g., per-step:
-      - name: Build the project with Nix
-        run: nix-build
+      - name: Build the project with Nix
+        if: runner.os != 'macOS'
+        run: nix-build
+      - name: Build the project with Nix (macOS, Bash 5)
+        if: runner.os == 'macOS'
+        shell: /usr/local/bin/bash -euo pipefail {0}
+        run: nix-build

Or set a job-level default shell for macOS via a matrix include. I can draft that if you prefer.


19-23: Add workflow/job concurrency to auto-cancel superseded runs on the same ref.

Saves CI minutes and queue time when force-pushing during PR iteration.

For example:

 jobs:
   tests:
+    concurrency:
+      group: ${{ github.workflow }}-${{ github.ref }}-${{ matrix.os }}
+      cancel-in-progress: true
     strategy:
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge Base: Disabled due to data retention organization setting

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 6efa88b and c7f7cab.

📒 Files selected for processing (1)
  • .github/workflows/test.yml (1 hunks)

Comment thread .github/workflows/test.yml Outdated
Adds Dependabot configuration for automatic dependency updates

Pins Nix action to specific version for better reproducibility
Configures additional substituters and trusted keys for IOG cache to improve build performance
@claude

claude Bot commented Aug 26, 2025

Copy link
Copy Markdown

Claude finished @NickSeagull's task —— View job


PR Review: Port NeoHaskell to haskell.nix Infrastructure ✅

I've thoroughly reviewed the migration to haskell.nix infrastructure. This is a well-executed migration that brings significant improvements to the build system. Here's my detailed analysis:

Review Checklist:

  • Read repository guidelines and understand project structure
  • Analyze migration to haskell.nix infrastructure
  • Review key file changes (haskell-project.nix, install.sh, common.nix)
  • Check for code quality and best practices
  • Identify potential bugs or issues
  • Assess security implications
  • Evaluate performance considerations
  • Review test coverage impact
  • Provide constructive feedback

✅ Strengths & Positive Changes

1. Excellent Architecture Design

  • cli/nix/haskell-project.nix:3-4: The new API { packages, mainPackageName, executableName } is much more flexible than the previous single-project approach
  • common.nix:4-12: Clean separation of concerns with packages defined as a map pointing to the same source (./.)

2. Robust Installation Script (scripts/install.sh)

  • scripts/install.sh:48-62: Smart configuration using /etc/nix/nix.custom.conf avoids conflicts with system configs
  • scripts/install.sh:68-93: Comprehensive cross-platform daemon restart logic covering macOS, Linux+systemd, and WSL
  • scripts/install.sh:56-61: Proper duplicate detection prevents config pollution

3. Consistent Build Infrastructure

  • Both NeoHaskell and user projects now use the same haskell.nix foundation
  • shell.nix:1 and default.nix:2: Clean delegation to common.nix eliminates code duplication

⚠️ Areas for Improvement

1. Security Considerations

  • cli/nix/haskell-project.nix:7-9: The haskell.nix revision is pinned (good!), but consider updating to a more recent commit for security patches
  • scripts/install.sh:6-7: Binary cache URLs are hardcoded. Consider validation or allowing override via environment variables

2. Error Handling & Robustness

  • cli/nix/haskell-project.nix:15-21: The builtins.listToAttrs and builtins.map combination could fail silently if packages contains invalid keys
  • cli/src/Neo/Build.hs:36 & cli/src/Neo/Shell.hs:36: Maybe.getOrDie should be replaced with proper error handling as noted in TODO
  • scripts/install.sh:71-92: Daemon restart failures are only warnings, but installation continues regardless

3. Code Quality Issues

  • cli/src/Neo/Build.hs:78-80 & cli/src/Neo/Shell.hs:78-80: Nix expressions are embedded as strings. Consider extracting to template files for better maintainability
  • common.nix:6-8: Both packages point to the same source (./.), which works but could be confusing. Consider adding comments explaining the monorepo structure

4. Performance & Build Considerations

  • cli/nix/haskell-project.nix:19: Using ghc910 is cutting-edge but may have stability issues. Document why this version is required
  • Missing: No obvious way to override compiler version per project

🔍 Potential Bugs

1. Path Resolution

  • cli/src/Neo/Build/Templates/Nix.hs:18: The template embeds haskellProject content directly. Ensure this works correctly when the CLI is installed system-wide
  • common.nix:3: Relative path ./cli/nix/haskell-project.nix may break if common.nix is used from different working directories

2. Error Propagation

  • cli/nix/haskell-project.nix:17-20: If any package in the packages attribute set fails to build, the entire build fails without clear indication of which package caused the issue

🚀 Performance Improvements

Positive Impact:

  • Binary cache integration will dramatically reduce build times
  • Shared haskell.nix infrastructure enables better caching between projects
  • Monorepo support allows building only changed components

Potential Optimizations:

  • Consider adding NIX_BUILD_CORES configuration for parallel builds
  • The installation script could check for existing cache configuration before attempting to add it

🧪 Test Coverage Considerations

Current State:

  • No new tests added for the Nix infrastructure changes
  • Existing Cabal tests should continue to work

Recommendations:

  • Add integration tests for nix-build and nix-shell operations
  • Test the installation script in different environments (macOS, Linux, WSL)
  • Verify that binary cache configuration actually speeds up builds

📝 Minor Suggestions

  1. Documentation: Consider adding a comment in common.nix explaining why both packages point to the same source
  2. Validation: Add input validation in haskell-project.nix for required parameters
  3. Logging: The installation script could be more verbose about what it's doing during the cache setup
  4. Version Consistency: Ensure the 0.9.1 version bump is coordinated across all deployment channels

🎯 Overall Assessment

This is a high-quality migration that significantly improves NeoHaskell's build infrastructure. The code follows good practices, the architecture is sound, and the installation improvements are substantial. The identified issues are mostly minor and don't affect the core functionality.

Recommendation: Approve

The benefits of unified infrastructure, better caching, and cross-platform installation fixes far outweigh the minor issues identified.


Removes unnecessary caching configuration and Hackage update step from the GitHub Actions workflow to simplify the CI pipeline since Nix already handles dependency management efficiently

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
.github/workflows/test.yml (2)

51-56: Gate “cabal update” behind the cache miss to shave minutes from CI.

The conditional is commented out; re‑enable to avoid redundant index updates when the Cabal cache hits.

-      - name: Update Cabal Hackage list
-        # if: steps.cache.outputs.cache-hit != 'true'
-        run: nix-shell --run "cabal update"
+      - name: Update Cabal Hackage list
+        if: steps.cache.outputs.cache-hit != 'true'
+        run: nix-shell --run "cabal update"

38-40: Optional: Prefer nix build over nix-build for forward-compat (matches haskell.nix migration).

Since the repo is moving to unified haskell.nix infra, consider nix build (flakes or default attr) for consistency and future deprecation safety.

-      - name: Build the project with Nix
-        run: nix-build
+      - name: Build the project with Nix
+        run: nix build .#package || nix-build
♻️ Duplicate comments (1)
.github/workflows/test.yml (1)

32-36: Good: action is now pinned to a tag; consider pinning to an exact commit for maximum immutability.

You addressed the prior supply‑chain warning by moving off a floating ref. To fully lock CI, pin to the release’s commit SHA (Dependabot can still bump it).

Example:

-      - uses: DeterminateSystems/determinate-nix-action@v3.8.6
+      - uses: DeterminateSystems/determinate-nix-action@<commit-sha-for-v3.8.6>

If you prefer tags, keep as-is and let Dependabot manage upgrades (now configured).

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge Base: Disabled due to data retention organization setting

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between c7f7cab and 5d95459.

📒 Files selected for processing (2)
  • .github/dependabot.yml (1 hunks)
  • .github/workflows/test.yml (1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
.github/dependabot.yml

[error] 6-6: no new line character at the end of file

(new-line-at-end-of-file)

⏰ 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). (3)
  • GitHub Check: tests (ubuntu-latest)
  • GitHub Check: tests (macos-latest)
  • GitHub Check: claude-review
🔇 Additional comments (1)
.github/workflows/test.yml (1)

32-36: Divine Edict: Verify IOHK Cache Key Pairing in CI

The workflow’s extra-conf block correctly references https://cache.iog.io and the hydra.iohk.io key, but our sandbox cannot install Nix to confirm. You must ensure on both Linux and macOS runners that the substituter and public-key actually load and match:

• File: .github/workflows/test.yml, in each job using DeterminateSystems/determinate-nix-action@…
– Confirm extra-substituters = https://cache.iog.io
– Confirm extra-trusted-public-keys = hydra.iohk.io:f/Ea+s+dFdN+3Y/G+FDgSq+a5NEWhJGzdjvKNGv0/EQ=

Add a verification step immediately after Nix is configured (before the build) in every runner job:

       - uses: DeterminateSystems/determinate-nix-action@v3.8.6
         with:
           extra-conf: |
             extra-substituters = https://cache.iog.io
             extra-trusted-public-keys = hydra.iohk.io:f/Ea+s+dFdN+3Y/G+FDgSq+a5NEWhJGzdjvKNGv0/EQ=

+      - name: Verify Nix cache substituter & key
+        run: |
+          nix show-config | grep -E 'substituters\s*=.*cache\.iog\.io' \
+            && echo "✔ substituter present" \
+            || { echo "✘ substituter missing"; exit 1; }
+          nix show-config | grep -E 'trusted-public-keys\s*=.*hydra\.iohk\.io:' \
+            && echo "✔ public key present" \
+            || { echo "✘ public key missing"; exit 1; }

Execute this on both your Linux and macOS jobs to prevent silent cache misses.

Comment thread .github/dependabot.yml
Comment on lines +1 to +6
version: 2
updates:
- package-ecosystem: github-actions
directory: /
schedule:
interval: weekly No newline at end of file

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Add a trailing newline to satisfy linters.

YAMLlint flags “no new line character at the end of file.” Add a newline to keep CI green and editors happy.

Apply:

-      interval: weekly
+      interval: weekly
+
📝 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.

Suggested change
version: 2
updates:
- package-ecosystem: github-actions
directory: /
schedule:
interval: weekly
# .github/dependabot.yml
version: 2
updates:
- package-ecosystem: github-actions
directory: /
schedule:
interval: weekly
🧰 Tools
🪛 YAMLlint (1.37.1)

[error] 6-6: no new line character at the end of file

(new-line-at-end-of-file)

🤖 Prompt for AI Agents
In .github/dependabot.yml around lines 1 to 6 the file is missing a trailing
newline which causes yamllint to fail; edit the file and add a single newline
character at the end of the file (ensure the file ends with a line break) then
save/commit so linters and CI see the newline.

🧹 Nitpick (assertive)

Tame update noise with groups and metadata (still one ecosystem: github-actions).

Given the PR’s goal to standardize infra, group GH Actions updates to reduce PR churn and add metadata for routing.

Example:

 version: 2
 updates:
   - package-ecosystem: github-actions
     directory: /
     schedule:
       interval: weekly
+    groups:
+      gha-all:
+        patterns:
+          - "*"
+    reviewers:
+      - "neo-haskell/maintainers"
+    labels:
+      - "dependencies"
+      - "gha"

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 YAMLlint (1.37.1)

[error] 6-6: no new line character at the end of file

(new-line-at-end-of-file)

🤖 Prompt for AI Agents
In .github/dependabot.yml lines 1-6, the config currently creates separate
weekly PRs for GitHub Actions updates; add a dependabot "groups" section to
bundle all github-actions updates into a single grouped PR and add metadata
fields (labels, reviewers or assignees, and an optional
open-pull-requests-limit) to the update entry so PRs are routed and
rate-limited; update the file by adding a top-level groups mapping that
names/combines the github-actions updates into one group and extend the existing
update entry with metadata keys like labels and reviewers/assignees and set
open-pull-requests-limit to control churn.

@claude

claude Bot commented Aug 26, 2025

Copy link
Copy Markdown

Claude finished @NickSeagull's task —— View job


PR Review: Port NeoHaskell to haskell.nix Infrastructure ✅

I've thoroughly reviewed the migration to haskell.nix infrastructure. This is a well-executed migration that brings significant improvements to the build system. Here's my detailed analysis:

Review Checklist:

  • Read repository guidelines and understand project structure
  • Analyze migration to haskell.nix infrastructure
  • Review key file changes (haskell-project.nix, install.sh, common.nix)
  • Check for code quality and best practices
  • Identify potential bugs or issues
  • Assess security implications
  • Evaluate performance considerations
  • Review test coverage impact
  • Provide constructive feedback

✅ Strengths & Positive Changes

1. Excellent Architecture Design

  • cli/nix/haskell-project.nix:3-4: The new API { packages, mainPackageName, executableName } is much more flexible than the previous single-project approach
  • common.nix:4-12: Clean separation of concerns with packages defined as a map pointing to the same source (./.)

2. Robust Installation Script (scripts/install.sh)

  • scripts/install.sh:48-62: Smart configuration using /etc/nix/nix.custom.conf avoids conflicts with system configs
  • scripts/install.sh:68-93: Comprehensive cross-platform daemon restart logic covering macOS, Linux+systemd, and WSL
  • scripts/install.sh:56-61: Proper duplicate detection prevents config pollution

3. Consistent Build Infrastructure

  • Both NeoHaskell and user projects now use the same haskell.nix foundation
  • shell.nix:1 and default.nix:2: Clean delegation to common.nix eliminates code duplication

⚠️ Areas for Improvement

1. Security Considerations

  • cli/nix/haskell-project.nix:7-9: The haskell.nix revision is pinned (good!), but consider updating to a more recent commit for security patches
  • scripts/install.sh:6-7: Binary cache URLs are hardcoded. Consider validation or allowing override via environment variables

2. Error Handling & Robustness

  • cli/nix/haskell-project.nix:15-21: The builtins.listToAttrs and builtins.map combination could fail silently if packages contains invalid keys
  • cli/src/Neo/Build.hs:36 & cli/src/Neo/Shell.hs:36: Maybe.getOrDie should be replaced with proper error handling as noted in TODO
  • scripts/install.sh:71-92: Daemon restart failures are only warnings, but installation continues regardless

3. Code Quality Issues

  • cli/src/Neo/Build.hs:78-80 & cli/src/Neo/Shell.hs:78-80: Nix expressions are embedded as strings. Consider extracting to template files for better maintainability
  • common.nix:6-8: Both packages point to the same source (./.), which works but could be confusing. Consider adding comments explaining the monorepo structure

4. Performance & Build Considerations

  • cli/nix/haskell-project.nix:19: Using ghc910 is cutting-edge but may have stability issues. Document why this version is required
  • Missing: No obvious way to override compiler version per project

🔍 Potential Bugs

1. Path Resolution

  • cli/src/Neo/Build/Templates/Nix.hs:18: The template embeds haskellProject content directly. Ensure this works correctly when the CLI is installed system-wide
  • common.nix:3: Relative path ./cli/nix/haskell-project.nix may break if common.nix is used from different working directories

2. Error Propagation

  • cli/nix/haskell-project.nix:17-20: If any package in the packages attribute set fails to build, the entire build fails without clear indication of which package caused the issue

🚀 Performance Improvements

Positive Impact:

  • Binary cache integration will dramatically reduce build times
  • Shared haskell.nix infrastructure enables better caching between projects
  • Monorepo support allows building only changed components

Potential Optimizations:

  • Consider adding NIX_BUILD_CORES configuration for parallel builds
  • The installation script could check for existing cache configuration before attempting to add it

🧪 Test Coverage Considerations

Current State:

  • No new tests added for the Nix infrastructure changes
  • Existing Cabal tests should continue to work

Recommendations:

  • Add integration tests for nix-build and nix-shell operations
  • Test the installation script in different environments (macOS, Linux, WSL)
  • Verify that binary cache configuration actually speeds up builds

📝 Minor Suggestions

  1. Documentation: Consider adding a comment in common.nix explaining why both packages point to the same source
  2. Validation: Add input validation in haskell-project.nix for required parameters
  3. Logging: The installation script could be more verbose about what it's doing during the cache setup
  4. Version Consistency: Ensure the 0.9.1 version bump is coordinated across all deployment channels

🎯 Overall Assessment

This is a high-quality migration that significantly improves NeoHaskell's build infrastructure. The code follows good practices, the architecture is sound, and the installation improvements are substantial. The identified issues are mostly minor and don't affect the core functionality.

Recommendation: Approve

The benefits of unified infrastructure, better caching, and cross-platform installation fixes far outweigh the minor issues identified.


@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
.github/workflows/test.yml (1)

32-33: Pinned Determinate Nix action — supply-chain win.

Switching to a fixed tag v3.8.6 resolves the prior floating-ref risk. This directly addresses the earlier review note about pinning. (github.com)

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge Base: Disabled due to data retention organization setting

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 5d95459 and 1614e83.

📒 Files selected for processing (1)
  • .github/workflows/test.yml (1 hunks)
⏰ 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). (3)
  • GitHub Check: tests (macos-latest)
  • GitHub Check: tests (ubuntu-latest)
  • GitHub Check: claude-review

Comment thread .github/workflows/test.yml
Comment on lines +34 to +36
extra-conf: |
extra-substituters = https://cache.iog.io
extra-trusted-public-keys = hydra.iohk.io:f/Ea+s+dFdN+3Y/G+FDgSq+a5NEWhJGzdjvKNGv0/EQ=

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Confirm extra-conf semantics; add a guard to prove caches are active.

Using extra-conf with extra-substituters and extra-trusted-public-keys is supported and appends to existing lists, but it’s order-dependent in nix.conf. Add a quick verification step to fail fast if the cache isn’t actually being used. (github.com, nix.dev)

Apply this minimal check right after the Determinate Nix step:

       - uses: DeterminateSystems/determinate-nix-action@v3.8.6
         with:
           extra-conf: |
             extra-substituters = https://cache.iog.io
             extra-trusted-public-keys = hydra.iohk.io:f/Ea+s+dFdN+3Y/G+FDgSq+a5NEWhJGzdjvKNGv0/EQ=
+      - name: Verify Nix cache wiring
+        run: |
+          nix --version
+          nix show-config | sed -n 's/^substituters = //p; s/^trusted-public-keys = //p'
+          nix show-config | grep -q 'https://cache.iog.io' || { echo "cache.iog.io not in substituters"; exit 1; }
+          nix show-config | grep -q 'hydra.iohk.io:' || { echo "IOG key not in trusted-public-keys"; exit 1; }
📝 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.

Suggested change
extra-conf: |
extra-substituters = https://cache.iog.io
extra-trusted-public-keys = hydra.iohk.io:f/Ea+s+dFdN+3Y/G+FDgSq+a5NEWhJGzdjvKNGv0/EQ=
extra-conf: |
extra-substituters = https://cache.iog.io
extra-trusted-public-keys = hydra.iohk.io:f/Ea+s+dFdN+3Y/G+FDgSq+a5NEWhJGzdjvKNGv0/EQ=
- name: Verify Nix cache wiring
run: |
nix --version
nix show-config | sed -n 's/^substituters = //p; s/^trusted-public-keys = //p'
nix show-config | grep -q 'https://cache.iog.io' || { echo "cache.iog.io not in substituters"; exit 1; }
nix show-config | grep -q 'hydra.iohk.io:' || { echo "IOG key not in trusted-public-keys"; exit 1; }
🤖 Prompt for AI Agents
In .github/workflows/test.yml around lines 34-36, the workflow appends
extra-substituters and extra-trusted-public-keys via extra-conf but doesn't
verify they took effect; add a new job step immediately after the Determinate
Nix step that programmatically checks nix configuration/substituters and fails
fast if the configured cache URL (https://cache.iog.io) is not present or not
active — e.g., invoke nix to output current substituters/config (JSON or text),
assert the cache URL appears in the substituters list, and exit nonzero if it
does not; keep the check minimal and CI-friendly so the workflow fails early
when the cache is not actually used.

Comments out doctest execution in CI workflow to prevent build failures

References issue #204
@claude

claude Bot commented Aug 26, 2025

Copy link
Copy Markdown

Claude finished @NickSeagull's task —— View job


PR Review: Port NeoHaskell to haskell.nix Infrastructure

I've thoroughly reviewed the migration to haskell.nix infrastructure. This is a well-executed migration that brings significant improvements to the build system. Here's my detailed analysis:

Review Checklist:

  • Read repository guidelines and understand project structure
  • Analyze migration to haskell.nix infrastructure
  • Review key file changes (haskell-project.nix, install.sh, common.nix)
  • Check for code quality and best practices
  • Identify potential bugs or issues
  • Assess security implications
  • Evaluate performance considerations
  • Review test coverage impact
  • Provide constructive feedback

✅ Strengths & Positive Changes

1. Excellent Architecture Design

  • cli/nix/haskell-project.nix:3-4: The new API { packages, mainPackageName, executableName } is much more flexible than the previous single-project approach
  • common.nix:4-12: Clean separation of concerns with packages defined as a map pointing to the same source (./.)

2. Robust Installation Script (scripts/install.sh)

  • scripts/install.sh:48-62: Smart configuration using /etc/nix/nix.custom.conf avoids conflicts with system configs
  • scripts/install.sh:68-93: Comprehensive cross-platform daemon restart logic covering macOS, Linux+systemd, and WSL
  • scripts/install.sh:56-61: Proper duplicate detection prevents config pollution

3. Consistent Build Infrastructure

  • Both NeoHaskell and user projects now use the same haskell.nix foundation
  • shell.nix:1 and default.nix:2: Clean delegation to common.nix eliminates code duplication

⚠️ Areas for Improvement

1. Security Considerations

  • cli/nix/haskell-project.nix:7-9: The haskell.nix revision is pinned (good!), but consider updating to a more recent commit for security patches
  • scripts/install.sh:6-7: Binary cache URLs are hardcoded. Consider validation or allowing override via environment variables

2. Error Handling & Robustness

  • cli/nix/haskell-project.nix:15-21: The builtins.listToAttrs and builtins.map combination could fail silently if packages contains invalid keys
  • cli/src/Neo/Build.hs:36 & cli/src/Neo/Shell.hs:36: Maybe.getOrDie should be replaced with proper error handling as noted in TODO
  • scripts/install.sh:71-92: Daemon restart failures are only warnings, but installation continues regardless

3. Code Quality Issues

  • cli/src/Neo/Build.hs:78-80 & cli/src/Neo/Shell.hs:78-80: Nix expressions are embedded as strings. Consider extracting to template files for better maintainability
  • common.nix:6-8: Both packages point to the same source (./.), which works but could be confusing. Consider adding comments explaining the monorepo structure

4. Performance & Build Considerations

  • cli/nix/haskell-project.nix:19: Using ghc910 is cutting-edge but may have stability issues. Document why this version is required
  • Missing: No obvious way to override compiler version per project

🔍 Potential Bugs

1. Path Resolution

  • cli/src/Neo/Build/Templates/Nix.hs:18: The template embeds haskellProject content directly. Ensure this works correctly when the CLI is installed system-wide
  • common.nix:3: Relative path ./cli/nix/haskell-project.nix may break if common.nix is used from different working directories

2. Error Propagation

  • cli/nix/haskell-project.nix:17-20: If any package in the packages attribute set fails to build, the entire build fails without clear indication of which package caused the issue

🚀 Performance Improvements

Positive Impact:

  • Binary cache integration will dramatically reduce build times
  • Shared haskell.nix infrastructure enables better caching between projects
  • Monorepo support allows building only changed components

Potential Optimizations:

  • Consider adding NIX_BUILD_CORES configuration for parallel builds
  • The installation script could check for existing cache configuration before attempting to add it

🧪 Test Coverage Considerations

Current State:

  • No new tests added for the Nix infrastructure changes
  • Existing Cabal tests should continue to work

Recommendations:

  • Add integration tests for nix-build and nix-shell operations
  • Test the installation script in different environments (macOS, Linux, WSL)
  • Verify that binary cache configuration actually speeds up builds

📝 Minor Suggestions

  1. Documentation: Consider adding a comment in common.nix explaining why both packages point to the same source
  2. Validation: Add input validation in haskell-project.nix for required parameters
  3. Logging: The installation script could be more verbose about what it's doing during the cache setup
  4. Version Consistency: Ensure the 0.9.1 version bump is coordinated across all deployment channels

🎯 Overall Assessment

This is a high-quality migration that significantly improves NeoHaskell's build infrastructure. The code follows good practices, the architecture is sound, and the installation improvements are substantial. The identified issues are mostly minor and don't affect the core functionality.

Recommendation: Approve

The benefits of unified infrastructure, better caching, and cross-platform installation fixes far outweigh the minor issues identified.


View job

@claude

claude Bot commented Aug 26, 2025

Copy link
Copy Markdown

Claude finished @NickSeagull's task —— View job


PR Review: Port NeoHaskell to haskell.nix Infrastructure ✅

I've thoroughly reviewed the migration to haskell.nix infrastructure. This is a well-executed migration that brings significant improvements to the build system. Here's my detailed analysis:

Review Checklist:

  • Read repository guidelines and understand project structure
  • Analyze migration to haskell.nix infrastructure
  • Review key file changes (haskell-project.nix, install.sh, common.nix)
  • Check for code quality and best practices
  • Identify potential bugs or issues
  • Assess security implications
  • Evaluate performance considerations
  • Review test coverage impact
  • Provide constructive feedback

✅ Strengths & Positive Changes

1. Excellent Architecture Design

  • cli/nix/haskell-project.nix:3-4: The new API { packages, mainPackageName, executableName } is much more flexible than the previous single-project approach
  • common.nix:4-12: Clean separation of concerns with packages defined as a map pointing to the same source (./.)

2. Robust Installation Script (scripts/install.sh)

  • scripts/install.sh:48-62: Smart configuration using /etc/nix/nix.custom.conf avoids conflicts with system configs
  • scripts/install.sh:68-93: Comprehensive cross-platform daemon restart logic covering macOS, Linux+systemd, and WSL
  • scripts/install.sh:56-61: Proper duplicate detection prevents config pollution

3. Consistent Build Infrastructure

  • Both NeoHaskell and user projects now use the same haskell.nix foundation
  • shell.nix:1 and default.nix:2: Clean delegation to common.nix eliminates code duplication

⚠️ Areas for Improvement

1. Security Considerations

  • cli/nix/haskell-project.nix:7-9: The haskell.nix revision is pinned (good!), but consider updating to a more recent commit for security patches
  • scripts/install.sh:6-7: Binary cache URLs are hardcoded. Consider validation or allowing override via environment variables

2. Error Handling & Robustness

  • cli/nix/haskell-project.nix:15-21: The builtins.listToAttrs and builtins.map combination could fail silently if packages contains invalid keys
  • cli/src/Neo/Build.hs:36 & cli/src/Neo/Shell.hs:36: Maybe.getOrDie should be replaced with proper error handling as noted in TODO
  • scripts/install.sh:71-92: Daemon restart failures are only warnings, but installation continues regardless

3. Code Quality Issues

  • cli/src/Neo/Build.hs:78-80 & cli/src/Neo/Shell.hs:78-80: Nix expressions are embedded as strings. Consider extracting to template files for better maintainability
  • common.nix:6-8: Both packages point to the same source (./.), which works but could be confusing. Consider adding comments explaining the monorepo structure

4. Performance & Build Considerations

  • cli/nix/haskell-project.nix:19: Using ghc910 is cutting-edge but may have stability issues. Document why this version is required
  • Missing: No obvious way to override compiler version per project

🔍 Potential Bugs

1. Path Resolution

  • cli/src/Neo/Build/Templates/Nix.hs:18: The template embeds haskellProject content directly. Ensure this works correctly when the CLI is installed system-wide
  • common.nix:3: Relative path ./cli/nix/haskell-project.nix may break if common.nix is used from different working directories

2. Error Propagation

  • cli/nix/haskell-project.nix:17-20: If any package in the packages attribute set fails to build, the entire build fails without clear indication of which package caused the issue

🚀 Performance Improvements

Positive Impact:

  • Binary cache integration will dramatically reduce build times
  • Shared haskell.nix infrastructure enables better caching between projects
  • Monorepo support allows building only changed components

Potential Optimizations:

  • Consider adding NIX_BUILD_CORES configuration for parallel builds
  • The installation script could check for existing cache configuration before attempting to add it

🧪 Test Coverage Considerations

Current State:

  • No new tests added for the Nix infrastructure changes
  • Existing Cabal tests should continue to work

Recommendations:

  • Add integration tests for nix-build and nix-shell operations
  • Test the installation script in different environments (macOS, Linux, WSL)
  • Verify that binary cache configuration actually speeds up builds

📝 Minor Suggestions

  1. Documentation: Consider adding a comment in common.nix explaining why both packages point to the same source
  2. Validation: Add input validation in haskell-project.nix for required parameters
  3. Logging: The installation script could be more verbose about what it's doing during the cache setup
  4. Version Consistency: Ensure the 0.9.1 version bump is coordinated across all deployment channels

🎯 Overall Assessment

This is a high-quality migration that significantly improves NeoHaskell's build infrastructure. The code follows good practices, the architecture is sound, and the installation improvements are substantial. The identified issues are mostly minor and don't affect the core functionality.

Recommendation: Approve

The benefits of unified infrastructure, better caching, and cross-platform installation fixes far outweigh the minor issues identified.


View job

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
.github/workflows/test.yml (2)

7-17: Broaden path filters so CI triggers on infra updates (common.nix, default.nix, shell.nix, installer).

Your migration centralizes Nix logic (e.g., common.nix) and adjusts the installer. Current filters won’t run on edits outside core/cli/nix. Expand patterns or drop path filters.

     paths:
       - "core/**"
       - "cli/**"
       - "nix/**"
+      - "common.nix"
+      - "default.nix"
+      - "shell.nix"
+      - "flake.nix"
+      - "scripts/**"

Repeat for the push trigger below.


19-23: Constrain default token scope for defense-in-depth.

Explicitly set minimal permissions for the job.

 jobs:
   tests:
+    permissions:
+      contents: read
     strategy:
♻️ Duplicate comments (6)
.github/workflows/test.yml (1)

32-36: Pin CI action to an immutable commit SHA and verify cache wiring immediately after.

Tags pointing at moving tags (v3.8.6) are better than @main but still mutable. Pin to the exact commit for supply‑chain hardening. Also, add a quick guard to prove your extra substituter/key actually took effect. This was requested before and is still missing.

Apply:

-      - uses: DeterminateSystems/determinate-nix-action@v3.8.6
+      - uses: DeterminateSystems/determinate-nix-action@<commit-sha-of-v3.8.6>
         with:
           extra-conf: |
             extra-substituters = https://cache.iog.io
             extra-trusted-public-keys = hydra.iohk.io:f/Ea+s+dFdN+3Y/G+FDgSq+a5NEWhJGzdjvKNGv0/EQ=
+      - name: Verify Nix cache wiring
+        run: |
+          nix --version
+          nix show-config | sed -n 's/^substituters = //p; s/^trusted-public-keys = //p'
+          nix show-config | grep -q 'https://cache.iog.io' || { echo "cache.iog.io not in substituters"; exit 1; }
+          nix show-config | grep -q 'hydra.iohk.io:' || { echo "IOG key not in trusted-public-keys"; exit 1; }

If you prefer, I can fetch and propose the exact commit SHA for v3.8.6 in a follow‑up.

cli/nix/haskell-project.nix (5)

14-21: Parse error: unquoted attribute name 'haskell-nix'. Quote hyphenated identifiers.

Nix requires quoting hyphenated attribute names. This currently won’t evaluate. This was raised earlier and still applies.

-      (final: _prev:
-        builtins.listToAttrs (builtins.map (packageName: {
-          name = "${packageName}";
-          value = final.haskell-nix.cabalProject {
+      (final: _prev:
+        builtins.listToAttrs (builtins.map (packageName: {
+          name = packageName;
+          value = final."haskell-nix".cabalProject {
             src = packages.${packageName};
-            compiler-nix-name = "ghc910";
+            compiler-nix-name = "ghc910";
           };
         }) (builtins.attrNames packages)))

3-3: Parameterize compiler choice; avoid hard‑coding "ghc910".

Make the compiler configurable (defaulting to ghc910) to avoid future breakage when upgrading haskell.nix or testing different GHCs. Previously suggested; still outstanding.

-{ packages, mainPackageName, executableName }:
+{ packages, mainPackageName, executableName, compilerNixName ? "ghc910" }:
@@
-            compiler-nix-name = "ghc910";
+            compiler-nix-name = compilerNixName;

Also applies to: 19-20


14-21: Avoid top-level attr collisions; namespace generated packages and use getAttr.

Injecting per‑package attrs at the top‑level risks name clashes. Prefer a namespaced set and select via builtins.getAttr. Previously noted.

-    overlays = [
+    overlays = [
       haskellNix.overlay
-      (final: _prev:
-        builtins.listToAttrs (builtins.map (packageName: {
-          name = "${packageName}";
-          value = final."haskell-nix".cabalProject {
-            src = packages.${packageName};
-            compiler-nix-name = compilerNixName;
-          };
-        }) (builtins.attrNames packages)))
+      (final: _prev: {
+        neohaskellPackages =
+          builtins.listToAttrs (builtins.map (packageName: {
+            name = packageName;
+            value = final."haskell-nix".cabalProject {
+              src = packages.${packageName};
+              compiler-nix-name = compilerNixName;
+            };
+          }) (builtins.attrNames packages));
+      })
     ];
@@
-  pkg = pkgsWithOverlay."${mainPackageName}";
+  pkg = builtins.getAttr mainPackageName pkgsWithOverlay.neohaskellPackages;

Also applies to: 26-26


26-26: Prefer builtins.getAttr over string interpolation for dynamic selection.

More idiomatic and avoids interpolation edge cases. Previously suggested.

-  pkg = pkgsWithOverlay."${mainPackageName}";
+  pkg = builtins.getAttr mainPackageName pkgsWithOverlay;

(If you apply the namespacing change above, target pkgsWithOverlay.neohaskellPackages instead.)


36-43: Do not mix derivations from different nixpkgs instances in shell inputs.

shellFor is from pkgsWithOverlay; buildInputs should come from the same set to avoid ABI and evaluation surprises. Previously raised.

-    buildInputs = [
-      pkgs.nil
-      pkgs.nixfmt-classic
-      pkgs.nixpkgs-fmt
-      pkgs.pkg-config
-      pkgs.zlib
-    ];
+    buildInputs = with pkgsWithOverlay; [
+      nil
+      nixfmt-classic
+      nixpkgs-fmt
+      pkg-config
+      zlib
+    ];
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge Base: Disabled due to data retention organization setting

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 1614e83 and 58f8b22.

📒 Files selected for processing (2)
  • .github/workflows/test.yml (1 hunks)
  • cli/nix/haskell-project.nix (1 hunks)
⏰ 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). (3)
  • GitHub Check: tests (macos-latest)
  • GitHub Check: tests (ubuntu-latest)
  • GitHub Check: claude-review
🔇 Additional comments (1)
cli/nix/haskell-project.nix (1)

30-35: LGTM: sensible dev tools in shell (cabal, HLS, hlint, hspec-discover).

Tooling set looks balanced for the migration and aligns with the PR goals.

Comment on lines +38 to +39
- uses: DeterminateSystems/magic-nix-cache-action@v13

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Also pin Magic Nix Cache to a commit SHA.

Mitigate action supply‑chain risk by pinning to an exact commit, not a mutable tag.

-      - uses: DeterminateSystems/magic-nix-cache-action@v13
+      - uses: DeterminateSystems/magic-nix-cache-action@<commit-sha-of-v13>
📝 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.

Suggested change
- uses: DeterminateSystems/magic-nix-cache-action@v13
- uses: DeterminateSystems/magic-nix-cache-action@<commit-sha-of-v13>
🤖 Prompt for AI Agents
.github/workflows/test.yml around lines 38-39: the workflow currently references
DeterminateSystems/magic-nix-cache-action@v13 which is a mutable tag; replace
this with an exact commit SHA to pin the action. Locate the desired commit on
the DeterminateSystems/magic-nix-cache-action GitHub repo, copy the full commit
SHA (40 chars) and update the uses line to use @<commit-sha>; commit the change
and run the workflow to verify it still works.

Comment on lines +40 to +41
- name: Build the project with Nix
run: nix-build

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Increase build log visibility for flakier investigations.

Add -L to surface derivation logs in CI output.

-      - name: Build the project with Nix
-        run: nix-build
+      - name: Build the project with Nix
+        run: nix-build -L
📝 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.

Suggested change
- name: Build the project with Nix
run: nix-build
- name: Build the project with Nix
run: nix-build -L
🤖 Prompt for AI Agents
In .github/workflows/test.yml around lines 40 to 41, the CI build step runs
"nix-build" without the -L flag which hides derivation logs; update the step to
run "nix-build -L" so derivation logs are printed to the workflow output,
improving visibility for flaky or failing builds.

Comment on lines +49 to +51
# Doctests are failing - See issue #204
# - name: Run core doctests
# run: nix-shell --run "cabal repl --with-compiler=doctest --repl-options=--verbose core"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Keep doctests running but non-blocking to avoid drift.

Instead of commenting out, run them with continue-on-error so failures are visible but don’t gate merges while #204 is open.

-      # Doctests are failing - See issue #204
-      # - name: Run core doctests
-      #   run: nix-shell --run "cabal repl --with-compiler=doctest --repl-options=--verbose core"
+      - name: Run core doctests (non-blocking) # See issue #204
+        continue-on-error: true
+        run: nix-shell --run "cabal repl --with-compiler=doctest --repl-options=--verbose core"
📝 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.

Suggested change
# Doctests are failing - See issue #204
# - name: Run core doctests
# run: nix-shell --run "cabal repl --with-compiler=doctest --repl-options=--verbose core"
- name: Run core doctests (non-blocking) # See issue #204
continue-on-error: true
run: nix-shell --run "cabal repl --with-compiler=doctest --repl-options=--verbose core"
🤖 Prompt for AI Agents
In .github/workflows/test.yml around lines 49 to 51, the doctest step has been
commented out; restore the step instead of removing it and make it non-blocking
by adding continue-on-error: true to the step. Re-enable the "Run core doctests"
step with the same run command (nix-shell --run "cabal repl
--with-compiler=doctest --repl-options=--verbose core") and add
continue-on-error: true (and keep the step name) so failures are reported in the
workflow but do not fail the job while issue #204 is open.

Comment on lines 11 to 24
pkgsWithOverlay = import haskellNix.sources.nixpkgs-unstable {
overlays = [
haskellNix.overlay
(final: _prev: {
"${projectName}Project" = final.haskell-nix.cabalProject {
inherit src;
compiler-nix-name = "ghc910";
};
})
(final: _prev:
builtins.listToAttrs (builtins.map (packageName: {
name = "${packageName}";
value = final.haskell-nix.cabalProject {
src = packages.${packageName};
compiler-nix-name = "ghc910";
};
}) (builtins.attrNames packages)))
];
inherit (haskellNix) config;
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Lock imported nixpkgs system to match caller to prevent accidental cross-system evaluation.

Pass system = pkgs.system when importing nixpkgs-unstable to ensure consistency with the outer pkgs.

-  pkgsWithOverlay = import haskellNix.sources.nixpkgs-unstable {
+  pkgsWithOverlay = import haskellNix.sources.nixpkgs-unstable {
+    system = pkgs.system;
     overlays = [
       haskellNix.overlay
       (final: _prev:
         builtins.listToAttrs (builtins.map (packageName: {
           name = "${packageName}";
           value = final."haskell-nix".cabalProject {
             src = packages.${packageName};
             compiler-nix-name = compilerNixName;
           };
         }) (builtins.attrNames packages)))
     ];
     inherit (haskellNix) config;
   };
📝 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.

Suggested change
pkgsWithOverlay = import haskellNix.sources.nixpkgs-unstable {
overlays = [
haskellNix.overlay
(final: _prev: {
"${projectName}Project" = final.haskell-nix.cabalProject {
inherit src;
compiler-nix-name = "ghc910";
};
})
(final: _prev:
builtins.listToAttrs (builtins.map (packageName: {
name = "${packageName}";
value = final.haskell-nix.cabalProject {
src = packages.${packageName};
compiler-nix-name = "ghc910";
};
}) (builtins.attrNames packages)))
];
inherit (haskellNix) config;
};
pkgsWithOverlay = import haskellNix.sources.nixpkgs-unstable {
system = pkgs.system;
overlays = [
haskellNix.overlay
(final: _prev:
builtins.listToAttrs (builtins.map (packageName: {
name = "${packageName}";
value = final.haskell-nix.cabalProject {
src = packages.${packageName};
compiler-nix-name = "ghc910";
};
}) (builtins.attrNames packages)))
];
inherit (haskellNix) config;
};
🤖 Prompt for AI Agents
In cli/nix/haskell-project.nix around lines 11 to 24 the import of
haskellNix.sources.nixpkgs-unstable does not specify a system, which can cause
cross-system evaluation; update the import call to pass system = pkgs.system
(i.e. import haskellNix.sources.nixpkgs-unstable { system = pkgs.system;
overlays = [ ... ]; inherit (haskellNix) config; }) so the imported nixpkgs is
locked to the outer pkgs system.

Comment on lines +44 to +45
package = (pkg.flake { }).packages."${executableName}";
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick (assertive)

Fail early if executableName is wrong; add an assertion.

Guard against typos by asserting the package key exists in the flake outputs.

-  package = (pkg.flake { }).packages."${executableName}";
+  package =
+    let
+      flakeOut = pkg.flake { };
+      pkgsOut  = flakeOut.packages;
+    in
+      assert builtins.hasAttr executableName pkgsOut;
+      pkgsOut."${executableName}";
📝 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.

Suggested change
package = (pkg.flake { }).packages."${executableName}";
}
package =
let
flakeOut = pkg.flake { };
pkgsOut = flakeOut.packages;
in
assert builtins.hasAttr executableName pkgsOut;
pkgsOut."${executableName}";
}
🤖 Prompt for AI Agents
In cli/nix/haskell-project.nix around lines 44-45, the code directly indexes the
flake outputs with package = (pkg.flake { }).packages."${executableName}"; which
silently fails if executableName is misspelled; add a precondition using
builtins.hasAttr (or compute the packages attrset first) and assert that the key
exists before indexing, then use that validated attrset to select the package so
the build fails early with a clear assertion message when the package key is
missing.

@NickSeagull NickSeagull merged commit 0df7983 into main Aug 26, 2025
10 checks passed
@NickSeagull NickSeagull deleted the chore/migrate-to-haskellnix branch August 26, 2025 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant