libfetchers: Add an fh-resolve input scheme#577
Open
edolstra wants to merge 3 commits into
Open
Conversation
This fetcher resolves a flakeref like
{
type = "fh-resolve";
org = "DeterminateSystems";
project = "nix-wasm-rust";
version = "^0";
output = "packages.x86_64-linux.default";
}
to a prebuilt store path by calling 'fh resolve', and substitutes that
path. This provides a convenient way for flakes to depend on prebuilt
binary artifacts (such as WASM plugins).
The lock file records the resolved store path and its NAR hash. Since
the store path is generally input-addressed, it cannot be recomputed
from the NAR hash, so Input::computeStorePath() now returns a recorded
'storePath' attribute directly. This makes the generic locked
fast-path (store reuse and substitution) operate on the resolved path,
so locked fetches never invoke 'fh'. Because a valid input-addressed
path does not imply the expected contents, the store accessor now
verifies the locked NAR hash against the path's actual NAR hash.
The resolved store paths are not allowed to have references, since
libfetchers trees have no concept of references.
Assisted-by: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds the ChangesFlakeHub resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Caller
participant FhResolveInputScheme
participant FileTransfer
participant FlakeHubAPI
participant NixStore
Caller->>FhResolveInputScheme: Request accessor for output reference
FhResolveInputScheme->>FileTransfer: Request output metadata
FileTransfer->>FlakeHubAPI: Fetch output endpoint
FlakeHubAPI-->>FileTransfer: Return store_path JSON
FileTransfer-->>FhResolveInputScheme: Return store path
FhResolveInputScheme->>NixStore: Query path info
NixStore-->>FhResolveInputScheme: Return narHash and references
FhResolveInputScheme-->>Caller: Return validated store accessor
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…Hash() Assisted-by: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/libfetchers/fh-resolve.cc (1)
1-8: 📐 Maintainability & Code Quality | 🔵 TrivialRun the formatter from the Nix dev shell
./maintainers/format.shexpectspre-commit; run it asnix develop -c ./maintainers/format.sh.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libfetchers/fh-resolve.cc` around lines 1 - 8, Run the repository formatter from the Nix development shell using nix develop -c ./maintainers/format.sh so the required pre-commit dependency is available.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/libfetchers/fetchers.cc`:
- Around line 313-327: Preserve the no-references invariant on the locked-path
fast path by adding a scheme-specific validation hook and invoking it before the
generic accessor is created in src/libfetchers/fetchers.cc:313-327. Keep direct
recorded-path reuse conditional on successful validation in
src/libfetchers/fetchers.cc:445-449, and expose the fh-resolve no-reference
check from its scheme implementation in src/libfetchers/fh-resolve.cc:186-195 so
referenced paths are rejected even when their NAR hash matches.
---
Nitpick comments:
In `@src/libfetchers/fh-resolve.cc`:
- Around line 1-8: Run the repository formatter from the Nix development shell
using nix develop -c ./maintainers/format.sh so the required pre-commit
dependency is available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5a9d25a4-8543-4988-b5bf-8f5bd17cf03f
📒 Files selected for processing (3)
src/libfetchers/fetchers.ccsrc/libfetchers/fh-resolve.ccsrc/libfetchers/meson.build
Comment on lines
+313
to
+327
| auto narHash = store.queryPathInfo(*storePath)->narHash; | ||
|
|
||
| /* If the store path is input-addressed (i.e. it comes from a | ||
| `storePath` attribute rather than being computed from the | ||
| NAR hash), its validity does not imply that it has the | ||
| expected contents, so verify the NAR hash. */ | ||
| if (narHash != *getNarHash()) | ||
| throw Error( | ||
| (unsigned int) 102, | ||
| "NAR hash mismatch in input '%s' at '%s', expected '%s' but got '%s'", | ||
| to_string(), | ||
| store.printStorePath(*storePath), | ||
| getNarHash()->to_string(HashFormat::SRI, true), | ||
| narHash.to_string(HashFormat::SRI, true)); | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve the no-references invariant on the locked-path fast path.
A final fh-resolve input with a recorded storePath bypasses FhResolveInputScheme::getAccessor(), so a referenced path with a matching NAR hash is accepted despite the PR’s required no-references invariant. Validate references before returning the generic accessor, preferably through a scheme validation hook.
src/libfetchers/fetchers.cc#L313-L327: invoke scheme-specific validation, or reject non-empty references forfh-resolve, before creating the accessor.src/libfetchers/fetchers.cc#L445-L449: keep direct recorded-path reuse conditional on that validation.src/libfetchers/fh-resolve.cc#L186-L195: expose the no-reference check so the generic locked-input path can apply it.
📍 Affects 2 files
src/libfetchers/fetchers.cc#L313-L327(this comment)src/libfetchers/fetchers.cc#L445-L449src/libfetchers/fh-resolve.cc#L186-L195
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/libfetchers/fetchers.cc` around lines 313 - 327, Preserve the
no-references invariant on the locked-path fast path by adding a scheme-specific
validation hook and invoking it before the generic accessor is created in
src/libfetchers/fetchers.cc:313-327. Keep direct recorded-path reuse conditional
on successful validation in src/libfetchers/fetchers.cc:445-449, and expose the
fh-resolve no-reference check from its scheme implementation in
src/libfetchers/fh-resolve.cc:186-195 so referenced paths are rejected even when
their NAR hash matches.
…irectly Instead of shelling out to 'fh resolve', do the FlakeHub JSON query ourselves. Credentials for api.flakehub.com are picked up from the user's netrc file, which the curl wrapper applies automatically. Assisted-by: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
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 (3)
src/libfetchers/fh-resolve.cc (3)
64-73: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject empty required attributes.
Attribute-based inputs can bypass the URL checks and supply empty
org,project,version, oroutput, producing malformed API requests. Enforce non-empty values here so URL and attribute inputs share the same contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libfetchers/fh-resolve.cc` around lines 64 - 73, Update inputFromAttrs to validate that the required org, project, version, and output attributes returned by getStrAttr are non-empty before constructing and returning Input; reject invalid attribute-based inputs using the same failure behavior as the URL parsing path.
28-31: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the scheme description to match the implementation.
This says the scheme uses
fh resolve, but the implementation queries the FlakeHub API directly. The current wording can mislead users about the required dependency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libfetchers/fh-resolve.cc` around lines 28 - 31, Update the schemeDescription() return text to describe direct FlakeHub API resolution instead of invoking `fh resolve`, while preserving the documented FlakeHub output reference format and avoiding any implication that the CLI dependency is required.
76-83: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject
url.authorityhere.fh-resolve://host/org/project/version#outputstill carrieshostinurl.authority, but this code only reads the path and silently drops it. This scheme has no host component, so reject any non-empty authority instead of resolving a different input.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/libfetchers/fh-resolve.cc` around lines 76 - 83, Update inputFromURL to reject URLs with a non-empty url.authority before processing pathSegments. Preserve the existing scheme and path validation, and throw BadURL for any authority so fh-resolve URLs cannot silently ignore a host component.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/libfetchers/fh-resolve.cc`:
- Line 169: Update the JSON parsing flow in the fetcher around the
nlohmann::json::parse call to catch parse_error, out_of_range, and type_error
exceptions, then rethrow the project’s fetcher-specific Error with descriptive
context that includes store_path and preserves the original exception details.
Keep the existing download and successful parsing behavior unchanged.
---
Outside diff comments:
In `@src/libfetchers/fh-resolve.cc`:
- Around line 64-73: Update inputFromAttrs to validate that the required org,
project, version, and output attributes returned by getStrAttr are non-empty
before constructing and returning Input; reject invalid attribute-based inputs
using the same failure behavior as the URL parsing path.
- Around line 28-31: Update the schemeDescription() return text to describe
direct FlakeHub API resolution instead of invoking `fh resolve`, while
preserving the documented FlakeHub output reference format and avoiding any
implication that the CLI dependency is required.
- Around line 76-83: Update inputFromURL to reject URLs with a non-empty
url.authority before processing pathSegments. Preserve the existing scheme and
path validation, and throw BadURL for any authority so fh-resolve URLs cannot
silently ignore a host component.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 224de90a-3138-4aa4-b65b-7cb5db409802
📒 Files selected for processing (1)
src/libfetchers/fh-resolve.cc
| percentEncode(getStrAttr(input.attrs, "output")))); | ||
| request.headers = {{"Accept", "application/json"}}; | ||
|
|
||
| auto json = nlohmann::json::parse(getFileTransfer()->download(request).data); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant function and nearby context.
wc -l src/libfetchers/fh-resolve.cc
sed -n '130,220p' src/libfetchers/fh-resolve.cc
# Find other uses of download(request).data and store_path handling in this file.
rg -n 'store_path|download\(request\)\.data|nlohmann::json::parse|json\.at\(' src/libfetchers/fh-resolve.ccRepository: DeterminateSystems/nix-src
Length of output: 3526
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l src/libfetchers/fh-resolve.cc
sed -n '130,220p' src/libfetchers/fh-resolve.cc
rg -n 'store_path|download\(request\)\.data|nlohmann::json::parse|json\.at\(' src/libfetchers/fh-resolve.ccRepository: DeterminateSystems/nix-src
Length of output: 3526
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for exception wrapping around fetcher accessors and for similar JSON parsing patterns.
rg -n "try \{|catch \(.*json|catch \(const .*Error|nlohmann::json::parse|json\.at\(\"store_path\"\)" src lib
# Find the declaration/usage of getAccessorUnchecked and any generic fetcher wrappers.
rg -n "getAccessorUnchecked|getAccessor\(" src include
# Inspect nearby code in fetcher-related helpers if present.
fd -a ".*fetcher.*" src includeRepository: DeterminateSystems/nix-src
Length of output: 36041
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect fetcher exception handling and the accessor call path.
rg -n "getAccessorUnchecked|getAccessor\(|catch \(const nlohmann::json::|catch \(const Error &|catch \(const std::exception &|throw Error\(" src/libfetchers src/libstore src/libcmd src/nix
# Read the fetcher dispatch code that calls into input schemes.
sed -n '1,260p' src/libfetchers/fetchers.cc
# Inspect any nearby exception translation in fh-resolve or sibling fetchers.
sed -n '1,380p' src/libfetchers/git-lfs-fetch.ccRepository: DeterminateSystems/nix-src
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '285,420p' src/libfetchers/fetchers.cc
printf '\n--- git-lfs-fetch ---\n'
sed -n '220,340p' src/libfetchers/git-lfs-fetch.cc
printf '\n--- github ---\n'
sed -n '430,560p' src/libfetchers/github.ccRepository: DeterminateSystems/nix-src
Length of output: 16428
Wrap the JSON parse in fetcher-specific errors. A malformed or schema-changed response can throw nlohmann::json::parse_error, out_of_range, or type_error here and skip the usual Error context; catch those and rethrow a descriptive fetcher error around store_path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/libfetchers/fh-resolve.cc` at line 169, Update the JSON parsing flow in
the fetcher around the nlohmann::json::parse call to catch parse_error,
out_of_range, and type_error exceptions, then rethrow the project’s
fetcher-specific Error with descriptive context that includes store_path and
preserves the original exception details. Keep the existing download and
successful parsing behavior unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
This adds a new fetcher
fh-resolvethat fetches trees from FlakeHub by resolving a flakehub org/project/version and fetching the store path of a flake output, similar to thefh resolvecommand. For example, the following flake input fetches the pre-built WASM plugins from the nix-wasm-rust flake:This is useful because it avoids having to evaluate the
nix-wasm-rustflake, so it's a lot cheaper/faster and does not require exposing the source of the input flake.Note that the resolved store paths are not allowed to have references, since libfetchers trees have no concept of references.
Context
Summary by CodeRabbit
New Features
fh-resolve:input scheme.storePathandnarHashare available.Bug Fixes
storePathis already provided.storePath, ensuring consistent usage.