nix: fix several regressions related to git introduced in v1.7

[sandydoo]

Fixes several regressions in v1.7 to do with git.

Path resolution outside of git repos

Nix 2.30 introduced an issue where local paths were copied to the Nix store when not part of a git repo. This broke the caching system because, once copied to the store, Nix would lose track of the context by not using the file system accessor for paths.

Path resolution for non-flake git+file inputs

Some people uses git+file inputs to work around flake limitations and compose shared devenvs in a monorepo. Nix 2.30 broke the path resolution here as well, causing the paths to be resolved to the Nix store.

Testing cachix/nix#7.

Fixes #1990.
Fixes #2002.
Fixes #2001.

Likely related (missing more information from OPs)

#1998

[claude]

Claude finished @sandydoo's task —— View job

---

PR Review Complete

I've reviewed the changes and have feedback on code quality, potential issues, and test coverage.

Todo List:

• Read and analyze all changed files
• Review devenv-run-tests modifications
• Evaluate new test infrastructure
• Check flake.lock updates
• Assess overall fix effectiveness
• Provide comprehensive review feedback

---

Issues Found

Critical Bug - Shebang Typo

File: tests/imports-monorepo-hack/.test.sh:1

#/usr/bin/env bash  # Missing ! - should be #!/usr/bin/env bash

This will cause the test to fail as bash won't be invoked properly.

Code Quality Issues

devenv-run-tests/src/main.rs:

1. Line 164-173: The git initialization could be more robust:

   if test_config.git_init {
       let git_init_status = Command::new("git")
           .arg("init")
           .arg("--initial-branch=main")
           .status()
           .into_diagnostic()?;
       // Consider adding git config for user.name/email to avoid warnings
   }

2. Line 150-158: The cp command could fail silently in some edge cases. Consider using fs::copy or adding better error context.

Test Coverage Concerns

Incomplete Edge Case Testing:

• The test only covers git+file://../ with flake: false
• Missing tests for:
  • Different relative path depths (../../, ../../../)
  • Absolute paths with git+file
  • Mixed scenarios with some inputs as git+file and others as regular paths
  • Error handling when git+file paths don't exist

Test Structure Issues:

• The test relies on use_shell: false which bypasses the devenv shell environment
• This might not catch issues that only occur within the full devenv context

Security Considerations

Path Traversal Risk:
The git+file:// URL pattern could potentially be exploited for path traversal attacks if not properly validated upstream. While this PR doesn't introduce the vulnerability, the test should validate that:

• Paths are properly sanitized
• Access is restricted to intended directories
• No symbolic link traversal occurs

Performance Impact

Flake.lock Updates:
The Nix dependency update to cachix/nix at devenv-2.30 looks appropriate and necessary for the fix. However:

• The lock file shows significant timestamp differences
• Consider documenting why this specific ref is needed
• Ensure the custom Nix build doesn't impact build times significantly

Recommendations

1. Fix the shebang immediately - this will cause test failures
2. Add more comprehensive tests covering additional git+file scenarios
3. Consider adding error handling tests to ensure graceful failures
4. Document the Nix dependency change in the commit message or PR description
5. Add integration with the shell environment to catch issues that only occur in the full devenv context

Overall Assessment

The approach is sound and addresses the core issues described in the PR. The addition of git_init configuration is a good design choice that provides testing flexibility. However, the test coverage could be more comprehensive, and the critical shebang bug needs immediate attention.

The flake.lock update appears necessary and appropriate for fixing the underlying Nix 2.30 path resolution issues.

---

[cloudflare-workers-and-pages]

Deploying devenv with    Cloudflare Pages

Latest commit:	8303f0b
Status:	✅  Deploy successful!
Preview URL:	https://31a82edc.devenv.pages.dev
Branch Preview URL:	https://fix-source-copying.devenv.pages.dev

View logs

[domenkozar]

Can we add a test so we can catch this next time?

[sandydoo]

Can we add a test so we can catch this next time?

Yeah, I'm thinking about how to do this.

We have integration tests for detecting files, but those use eval and this is a flake-specific issue.
And devenv-run-tests initializes a git repo to fix tests that use git-hooks, but that would fail to test for this issue.

[sandydoo]

Added #1995 to test this edge-case. I've cherry-picked the commit here, so the eval-cache tests should fail in #1995 and pass here.

[sandydoo]

Final test run.