Backward compatibility hack for getFlake applied to unsafeDiscardStringContext

[edolstra]

Motivation

Similar to #402 but with a different kind of string context.

Context

Summary by CodeRabbit

• Bug Fixes

  • Improved backward-compatibility handling for builtins.getFlake with path flake references, using a more direct store filesystem lookup approach.

• Tests

  • Added functional test verifying getFlake backward compatibility when applied to store-path string contexts.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR refactors builtins.getFlake's backward-compatibility handling for path flake references, replacing NixStringContext element scanning with direct store filesystem mount lookup. A functional test is added to verify the revised logic for discarded string contexts.

Changes

Cohort / File(s)	Summary
Flake Path Resolution Refactoring src/libflake/flake-primops.cc	Changed backward-compatibility logic from iterating NixStringContext elements to identify store paths, to directly querying the store filesystem via state.store->printStorePath wrapped in CanonPath. The refactored approach constructs the final flake path (including subdir) before invoking callFlake.
Backward-Compatibility Test tests/functional/flakes/get-flake.sh	Added functional test verifying getFlake works correctly with path:... ?narHash=... flake references that have had their string context discarded via builtins.unsafeDiscardStringContext.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• builtins.getFlake: Support path values #338: Modifies the same builtins.getFlake path-handling logic in flake-primops.cc and related test files.
• builtins.getFlake: Handle path:<p> where p has a discarded string context #402: Alters the identical code path for handling "path:..." flake references with discarded string contexts.
• builtins.getFlake fixes #337: Changes store-path and string context handling within builtins.getFlake in the same source file.

Suggested labels

flake-regression-test

Suggested reviewers

• cole-h

Poem

🐰 With store paths now mounted and queries quite bright,
The flakes are found swiftly—no context in sight!
We test the old paths with new clever ways,
The backward-compat dance deserves of our praise! ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: implementing a backward compatibility hack for getFlake when applied to unsafeDiscardStringContext, which aligns with the code modifications in both the implementation and test files.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch eelcodolstra/nix-371

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

tests/functional/flakes/get-flake.sh (1)

29-46: Consider adding a nested-path regression case.

This only exercises the empty subPath / empty dir case. Since the new branch now composes both the store subpath and flakeRef.subdir, a /subflake or ?dir=subflake variant would give much better coverage for path-joining regressions.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/functional/flakes/get-flake.sh` around lines 29 - 46, Add a nested-path
regression case that exercises composing a store subpath with flakeRef.subdir:
create a subdirectory (e.g., "$flake1Dir/subflake") containing a flake.nix that
mirrors the original but uses builtins.unsafeDiscardStringContext with a path
that includes the subpath (e.g.,
"path:/subflake:${self.sourceInfo}?narHash=${self.narHash}") or use the query
form "?dir=subflake", then evaluate the nested flake (e.g., nix eval --raw
"$flake1Dir/subflake#flakeOutputs.foo" or nix eval --raw
"$flake1Dir?dir=subflake#flakeOutputs.foo") and assert it equals "bar" to cover
the /subflake / ?dir=subflake variants alongside the existing empty-subPath
case.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/functional/flakes/get-flake.sh`:
- Around line 29-46: Add a nested-path regression case that exercises composing
a store subpath with flakeRef.subdir: create a subdirectory (e.g.,
"$flake1Dir/subflake") containing a flake.nix that mirrors the original but uses
builtins.unsafeDiscardStringContext with a path that includes the subpath (e.g.,
"path:/subflake:${self.sourceInfo}?narHash=${self.narHash}") or use the query
form "?dir=subflake", then evaluate the nested flake (e.g., nix eval --raw
"$flake1Dir/subflake#flakeOutputs.foo" or nix eval --raw
"$flake1Dir?dir=subflake#flakeOutputs.foo") and assert it equals "bar" to cover
the /subflake / ?dir=subflake variants alongside the existing empty-subPath
case.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 04063b19-a31e-4b47-8911-c78283c724df

📥 Commits

Reviewing files that changed from the base of the PR and between 0b5c51a and 274cc81.

📒 Files selected for processing (2)

• src/libflake/flake-primops.cc
• tests/functional/flakes/get-flake.sh

[github-actions]

🚀 Deployed on https://69d91b89f90c2b086c18ccc5--determinate-nix.netlify.app