libfetchers: Add an fh-resolve input scheme - #577
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>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds the ChangesFlakeHub resolution
Estimated code review effort: 3 (Moderate) | ~25 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 resolve endpoint
FlakeHubAPI-->>FileTransfer: Return store_path JSON
FileTransfer-->>FhResolveInputScheme: Return store path
FhResolveInputScheme->>NixStore: Query path info
NixStore-->>FhResolveInputScheme: Return narHash and references
FhResolveInputScheme->>FhResolveInputScheme: Validate hash 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>
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
…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>
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 runs a fake api.flakehub.com serving static resolve responses, with the resolved artifact baked into the VM's store. Assisted-by: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/nixos/fh-resolve.nix (1)
89-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffTest never exercises the actual substitution codepath.
virtualisation.additionalPathspre-seeds bothartifactandartifactWithRefsdirectly into the VM store, and substituters are force-disabled. Per thefh-resolve.cccontract (context snippet 1),getAccessoronly callsstore.ensurePath(*storePath)whenisValidPathis false — since the paths are always valid here, that branch (the "deduplicated store path substitution" logic referenced in the PR's commit summary) is never actually run by this end-to-end test.Since the commit message explicitly frames this as testing "a resolved artifact in the VM store," this may be intentional scope, but consider adding a follow-up scenario (e.g. serving the artifact from a local binary cache substituter without pre-seeding the store) to get real end-to-end coverage of the substitution 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 `@tests/nixos/fh-resolve.nix` around lines 89 - 96, Add a follow-up VM scenario that does not pre-seed artifact or artifactWithRefs via virtualisation.additionalPaths, while serving them from a local binary-cache substituter. Configure the test to resolve the artifact from that substituter so getAccessor reaches store.ensurePath when isValidPath is false, while preserving the existing pre-seeded scenario.
🤖 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.
Nitpick comments:
In `@tests/nixos/fh-resolve.nix`:
- Around line 89-96: Add a follow-up VM scenario that does not pre-seed artifact
or artifactWithRefs via virtualisation.additionalPaths, while serving them from
a local binary-cache substituter. Configure the test to resolve the artifact
from that substituter so getAccessor reaches store.ensurePath when isValidPath
is false, while preserving the existing pre-seeded scenario.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2387a63b-4e8c-4c51-a9ae-69acb2d5e94e
📒 Files selected for processing (3)
src/libfetchers/fh-resolve.cctests/nixos/default.nixtests/nixos/fh-resolve.nix
🚧 Files skipped from review as they are similar to previous changes (1)
- src/libfetchers/fh-resolve.cc
Factor the NAR hash and no-references checks into Input::checkStorePath(), shared between the generic store reuse/substitution path and the fh-resolve fetcher. Previously the generic path did not reject store paths with references. Assisted-by: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/libfetchers/fh-resolve.cc (1)
80-82: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject empty URL path segments.
pathSegments(/*skipEmpty=*/true)collapses repeated slashes, so malformed input can resolve a different FlakeHub reference than the user supplied. Preserve empty segments and reject them explicitly.Proposed fix
- auto path = url.pathSegments(/*skipEmpty=*/true) | std::ranges::to<std::vector<std::string>>(); - if (path.size() != 3) + auto path = url.pathSegments(/*skipEmpty=*/false) | std::ranges::to<std::vector<std::string>>(); + if (path.size() != 3 + || std::ranges::any_of(path, [](const auto & segment) { return segment.empty(); })) throw BadURL("URL '%s' should have the form 'fh-resolve:«org»/«project»/«version»#«output»'", url);🤖 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 80 - 82, Update the path extraction in the fh-resolve URL parsing flow to preserve empty segments instead of passing skipEmpty=true. In the surrounding path validation, explicitly reject any empty segment while retaining the requirement for exactly three segments before constructing the FlakeHub reference.
🤖 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.
Outside diff comments:
In `@src/libfetchers/fh-resolve.cc`:
- Around line 80-82: Update the path extraction in the fh-resolve URL parsing
flow to preserve empty segments instead of passing skipEmpty=true. In the
surrounding path validation, explicitly reject any empty segment while retaining
the requirement for exactly three segments before constructing the FlakeHub
reference.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b252152b-29cd-4eb7-8bc0-c19cb6fee01b
📒 Files selected for processing (3)
src/libfetchers/fetchers.ccsrc/libfetchers/fh-resolve.ccsrc/libfetchers/include/nix/fetchers/fetchers.hh
🚧 Files skipped from review as they are similar to previous changes (1)
- src/libfetchers/fetchers.cc
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
fh-resolve:input scheme, producing substituted/prebuilt store paths.storePathandnarHashare provided, and the resolvednarHashis persisted for reuse.storePath, with clearer “NAR hash mismatch” errors.fh-resolveresolution, locking behavior, and failure cases.