scripts: fix the packages option

[sandydoo]

The packages were being made available to the build script for the script, not the script itself.
I've also added a test for this in examples/scripts.

Additionally:

The runCommand script looked suspiciously similar to writeShellScript.

To pass PATH correctly, I had to use a wrapper script. Another option would be env, but that messes with exit codes, signals, and what not. Open to alternatives.

The scripts in examples/scripts are now tested in CI. I replaced the curl example to test accessing top-level packages without making external requests.

[claude]

Claude finished @sandydoo's task —— View job

---

Code Review Complete

Todo List:

• Read and analyze changed files
• Review code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Review test coverage improvements
• Provide final review feedback

Review Feedback

✅ Bug Fix is Correct

The core issue is properly addressed. Previously, packages were available only during build time (buildInputs), but now they're correctly made available at runtime via PATH manipulation.

⚠️ Potential Issues & Improvements

1. PATH Security Concern (src/modules/scripts.nix:60)

export PATH=${pkgs.lib.makeBinPath config.packages}:$PATH

• Issue: Prepending to PATH could potentially shadow system binaries if malicious packages are provided
• Suggestion: Consider appending instead: export PATH=$PATH:${pkgs.lib.makeBinPath config.packages} or document this behavior

2. Shell Execution Context (src/modules/scripts.nix:59)

#!${lib.getExe pkgs.bashInteractive}

• Question: Why bashInteractive specifically? The original used the configured package's binary
• Suggestion: Consider if plain pkgs.bash would suffice, or document why interactive features are needed

3. Error Handling (src/modules/scripts.nix:61)

exec ${execScript} "$@"

• Enhancement: The wrapper doesn't preserve the exit code if PATH setup fails (though unlikely)
• Minor: Consider adding basic error checking before exec

4. Performance Consideration

• Impact: The new approach creates an additional wrapper script layer (bash → actual script)
• Assessment: Minimal performance impact for typical use cases, but adds one more process

✅ Test Coverage Improvements

Excellent additions in examples/scripts/devenv.nix:48-61:

• Tests now verify that packages work correctly in both top-level and script-specific contexts
• Removed external dependency (curl) making tests more reliable
• Added comprehensive test coverage for different script types

Smart test cases:

• silly-example: Tests top-level packages (jq) access ✓
• serious-example: Tests script-specific packages (cowsay) ✓
• python-hello & nushell-greet: Tests different interpreters ✓

✅ Code Quality

Good practices:

• Clear separation of concerns with wrapper script
• Proper use of Nix functions (lib.getExe, pkgs.lib.makeBinPath)
• Maintained backward compatibility

Minor style notes:

• Code is well-structured and follows Nix conventions
• Good use of lib.hiPrioSet to maintain script precedence

🎯 Overall Assessment

This is a solid fix that correctly addresses the core issue. The wrapper approach is clean and the test improvements significantly increase confidence in the functionality. The security consideration around PATH ordering is the main point to address.

Recommendation: Approve with consideration of the PATH ordering discussion.

---

[github-actions]

🔍 Suggested Reviewers

Based on git blame analysis of the changed lines, the following contributors have significant experience with the modified code:

• @domenkozar - 72.7% of changed lines (16 lines)
• @mmlb - 27.3% of changed lines (6 lines)

Please consider reviewing this PR as you have authored significant portions of the code being modified. Your expertise would be valuable! 🙏

This comment was automatically generated by git-blame-auto-reviewer

Last updated: 2025-09-02T19:04:23.758Z

[cloudflare-workers-and-pages]

Deploying devenv with    Cloudflare Pages

Latest commit:	c50299c
Status:	🚫  Build failed.

View logs