Conversation
📝 WalkthroughWalkthroughIntroduce a provenance subsystem and thread provenance metadata through fetchers, source accessors, eval/flake, store write/copy flows, worker/daemon protocol, DB schema, JSON (de)serialization, public APIs, and functional tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Daemon
participant Store
participant Provenance
Client->>Daemon: AddToStoreFromDump(dump, ..., provenanceJSON?)
Daemon->>Store: addToStoreFromDump(..., provenance)
Note right of Store: create ValidPathInfo\ninfo.provenance = provenance
Store->>Provenance: wrap/chain provenance (Build/Copied)
Store-->Daemon: return path + provenance (if negotiated)
Daemon-->Client: response (path + provenance if negotiated)
sequenceDiagram
participant Fetcher
participant Input
participant Accessor
participant Store
participant Provenance
Fetcher->>Input: fetchToStore(...)
Input-->>Accessor: accessor (attrs)
Accessor->>Provenance: accessor.provenance = TreeProvenance(input.attrs)
Fetcher->>Store: addToStoreFromDump(..., provenance = accessor.getProvenance(path))
Store->>Provenance: record BuildProvenance/CopiedProvenance chain
Store-->Fetcher: store path with provenance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/libstore/dummy-store.cc (1)
235-304: Provenance parameter is accepted but not used inaddToStoreFromDump.The
provenanceparameter is added to the signature but is never used in the implementation. TheValidPathInfocreated on lines 276–288 and stored incontentsdoesn't include the provenance. Other stores handle this correctly:BinaryCacheStore::addToStoreFromDump(line 382) andLocalStore::addToStoreFromDump(line 1331) both assigninfo.provenance = provenance.Add the provenance assignment:
Suggested fix
auto info = ValidPathInfo::makeFromCA( *this, name, ContentAddressWithReferences::fromParts( hashMethod, std::move(hash), { .others = references, // caller is not capable of creating a self-reference, because // this is content-addressed without modulus .self = false, }), std::move(narHash.first)); info.narSize = narHash.second.value(); + info.provenance = std::move(provenance);
🤖 Fix all issues with AI agents
In `@src/libexpr/include/nix/expr/eval.hh`:
- Around line 1134-1147: Members are declared in the wrong order which breaks
the intended destruction order: move std::shared_ptr<const Provenance>
rootProvenance so it is declared before ref<Executor> executor (or alternatively
move ref<Executor> executor to be the final data member) to ensure executor is
destroyed last; update the declarations around the class fields that contain
rootProvenance, executor and the public methods
setRootProvenance/getRootProvenance accordingly so destructor ordering is
preserved.
In `@src/libfetchers/filtering-source-accessor.cc`:
- Around line 71-76: FilteringSourceAccessor::getProvenance currently bypasses
checkAccess() and the SubpathProvenance wrapping from
SourceAccessor::getProvenance, so update this method to first call
checkAccess(path) (or the appropriate access-checking call used by
SourceAccessor), then if the local provenance member exists return it, otherwise
obtain the child provenance via next->getProvenance(prefix / path) and wrap that
result in a SubpathProvenance constructed with the returned provenance and the
prefix before returning; also add an `#include` for the SubpathProvenance header
if not already present.
In `@src/libfetchers/tarball.cc`:
- Around line 106-109: The code currently stores the raw URL in provenance
(info.provenance = std::make_shared<FetchurlProvenance>(url)) which can leak
credentials or signed tokens; before creating FetchurlProvenance and calling
store.addToStore(info, source, ...), sanitize/redact the URL (remove userinfo
and sensitive query params) using the existing redaction helper if available (or
add a small helper that strips user:pass and known signed token query keys),
then pass the redacted string into FetchurlProvenance instead of the original
url so no secrets are persisted in narinfo/metadata.
- Around line 12-34: Add a Provenance::Register for the FetchurlProvenance type
so deserialization of {"type":"fetchurl"} returns a FetchurlProvenance instance:
register a Provenance::Register (e.g. registerFetchurlProvenance) that uses
getObject(json), valueAt(obj, "url"), getString(...) and
make_ref<FetchurlProvenance>(...) to construct the object; place this
registration in this file (or src/libfetchers/provenance.cc) so the type is
recognized at load time. Also sanitize/redact sensitive components of the url
before storing/serializing in FetchurlProvenance::to_json (e.g. strip
credentials or tokens) to avoid leaking credentials in provenance.
In `@src/libflake/include/nix/flake/flake.hh`:
- Around line 98-101: Flake::~Flake() in flake.cc needs the complete type for
Provenance because it destroys the std::shared_ptr<const Provenance> member
provenance; add `#include` "nix/flake/provenance.hh" to src/libflake/flake.cc so
the Provenance definition is visible when instantiating the destructor for Flake
(referencing the provenance member and the Flake::~Flake() destructor).
In `@src/libstore/include/nix/store/path-info.hh`:
- Around line 127-132: The header now exposes std::shared_ptr via the member
"provenance" (type std::shared_ptr<const Provenance>), so add a direct include
for <memory> at the top of this header to avoid relying on transitive includes;
ensure the include is placed with the other standard headers and before any code
that references std::shared_ptr/Provenance.
In `@src/libstore/include/nix/store/provenance.hh`:
- Around line 9-36: DerivationProvenance is missing a from_json registration so
Provenance::from_json() cannot reconstruct derivation nodes; add a registration
call analogous to CopiedProvenance using Provenance::Register to register
"derivation" (or the existing type tag) with a factory that constructs a
DerivationProvenance from JSON, ensuring the registration happens during module
init (the same translation unit where other registrations like CopiedProvenance
are registered) so Provenance::from_json can deserialize DerivationProvenance
objects.
In `@src/libstore/nar-info.cc`:
- Around line 134-136: The text serialization in NarInfo (in
src/libstore/nar-info.cc) appends provenance unconditionally; update the
NarInfo::to_string (or the function that builds `res`) to only append
"Provenance: " + provenance->to_json_str() when
experimentalFeatureSettings.isEnabled(Xp::Provenance) is true, mirroring the
gating used during parsing and the UnkeyedValidPathInfo::toJSON() behavior;
locate the block that currently does `if (provenance) res += "Provenance: " +
provenance->to_json_str() + "\n";` and change it to check the feature flag
before using `provenance`.
In `@src/libutil/experimental-features.cc`:
- Around line 323-330: The provenance experimental-feature entry currently has
an empty .trackingUrl which leads to a broken link in documentation; update the
documentation generation (documentExperimentalFeatures) to check
feature.trackingUrl (or equivalent field) and only emit the tracking/issue link
when it is non-empty, or alternatively set a real URL for the Xp::Provenance
entry; reference the Xp::Provenance feature record (name "provenance") and the
trackingUrl field when making the change so the link is suppressed for empty
values.
🧹 Nitpick comments (5)
src/libstore/include/nix/store/remote-store.hh (1)
91-99: Consider restoring default arguments for RemoteStore callers.C++ default args aren’t inherited, so removing them here can break direct
RemoteStoreusages even ifStorestill provides defaults. If this isn’t intentional, consider mirroring the base defaults to preserve API ergonomics.♻️ Proposed adjustment
- FileSerialisationMethod dumpMethod, - ContentAddressMethod hashMethod, - HashAlgorithm hashAlgo, - const StorePathSet & references, - RepairFlag repair, - std::shared_ptr<const Provenance> provenance) override; + FileSerialisationMethod dumpMethod = FileSerialisationMethod::NixArchive, + ContentAddressMethod hashMethod = ContentAddressMethod::Raw::NixArchive, + HashAlgorithm hashAlgo = HashAlgorithm::SHA256, + const StorePathSet & references = StorePathSet(), + RepairFlag repair = NoRepair, + std::shared_ptr<const Provenance> provenance = nullptr) override;src/libstore/provenance.cc (1)
6-25: Inconsistentnextfield serialization between provenance types.
DerivationProvenance::to_json()always includes the"next"field (asnullwhen absent), whileCopiedProvenance::to_json()only includes it when present. This inconsistency may cause confusion or issues during deserialization.Consider making the serialization consistent. Based on the
FlakeProvenancepattern fromsrc/libflake/provenance.cc, which always emits"next", aligning with that approach would be more consistent:♻️ Suggested fix for consistency
nlohmann::json CopiedProvenance::to_json() const { - nlohmann::json j{ + return nlohmann::json{ {"type", "copied"}, {"from", from}, + {"next", next ? next->to_json() : nlohmann::json(nullptr)}, }; - if (next) - j["next"] = next->to_json(); - return j; }src/libstore/build/derivation-building-goal.cc (1)
443-447: Consider capturing provenance for the AlreadyValid shortcut.
Provenance is fetched after the early return at Line 349, soAlreadyValidresults will report null provenance even when it exists. If you want consistent provenance in build results, consider querying it before that shortcut or in the AlreadyValid branch.src/libstore/store-api.cc (1)
888-895: Consider returning existing path info on the early-valid shortcut.
With the newnullptrreturn when the destination already has the path (Line 893–895), callers that want provenance (or other info) need an extra query. If that data is important in the “already present” case, consider returningdstStore.queryPathInfo(storePath)or explicitly documenting the contract.Also applies to: 907-942
src/libutil/include/nix/util/provenance.hh (1)
12-38: Add a virtual destructor toProvenanceas best practice.
While the base class is polymorphic and owns derived types throughstd::shared_ptr(which correctly captures concrete type deletes at construction), explicitly marking a polymorphic base class with a virtual destructor clarifies intent and ensures safety if the code evolves. This follows standard C++ polymorphism conventions.Proposed fix
struct Provenance { + virtual ~Provenance() = default; static ref<const Provenance> from_json_str(std::string_view);
edolstra
force-pushed
the
provenance-detsys
branch
6 times, most recently
from
fb14a66 to
e8599c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/libstore/include/nix/store/worker-protocol.hh (1)
66-84: Verify provenance handling in hardcoded protocol version 16 cases only—main negotiation path is correct.The primary code paths using
WorkerProto::BasicClientConnectionandWorkerProto::BasicServerConnectioncorrectly set theprovenanceflag based on negotiated features. However, three direct constructions with hardcodedversion = 16do not setprovenanceand will silently default tofalse:
src/libstore/store-api.cc:204inaddMultipleToStore()(marked FIXME)src/libstore/export-import.cc:147and:60for nario format import/exportThese are non-negotiated contexts marked as technical debt. If provenance should flow through these paths, they need explicit handling. Otherwise, document that these legacy/special-format paths intentionally bypass provenance.
src/libstore/include/nix/store/store-api.hh (1)
948-957: Handle nullptr return fromcopyStorePath.
The function can returnnullptrwhen the path already exists in the destination store. At least one call site insrc/libstore/build/substitution-goal.cc:258sets a promise value with the return directly without guarding againstnullptr, which could cause issues downstream.
🤖 Fix all issues with AI agents
In `@src/libstore/build/derivation-building-goal.cc`:
- Around line 443-447: The AlreadyValid fast‑path returns BuildResult::Success
before provenance is fetched, causing missing provenance; hoist the
maybeQueryPathInfo(drvPath) lookup (calling worker.evalStore.maybeQueryPathInfo
and capturing info->provenance into a std::shared_ptr<const Provenance>
provenance) before the early return for the AlreadyValid case and reuse that
provenance when constructing/returning BuildResult::Success so provenance is
present for the fast path as well as later build paths.
In `@src/libstore/dummy-store.cc`:
- Around line 235-243: The addToStoreFromDump implementation accepts a
provenance parameter but never attaches it to the created ValidPathInfo, so
provenance is dropped; update the function (addToStoreFromDump) to set
info.provenance = provenance (or equivalent member) on the ValidPathInfo
instance before calling whatever inserts the entry (e.g., insertPath /
_store.emplace or the existing insertion code) so the saved path records the
provided provenance.
In `@src/libstore/include/nix/store/legacy-ssh-store.hh`:
- Around line 76-79: The isUsefulProvenance() override currently returns true
but LegacySSHStore (ServeProto) cannot transmit provenance; change the method
implementation in the LegacySSHStore class so isUsefulProvenance() returns false
to accurately reflect protocol capabilities (or alternatively implement
provenance support in ServeProto/ReadConn/WriteConn and UnkeyedValidPathInfo
before keeping true) — locate the isUsefulProvenance() override and update its
return value to false.
In `@src/libutil/include/nix/util/provenance.hh`:
- Around line 3-8: Add explicit standard headers for the used types: include
<map>, <string>, <string_view>, and <memory> at the top of the header so that
uses of std::map, std::string, std::string_view, and std::shared_ptr are no
longer reliant on transitive includes; update the include block near the
existing includes ("nix/util/ref.hh", "nix/util/canon-path.hh", <functional>,
<nlohmann/json_fwd.hpp>) to add these four headers.
♻️ Duplicate comments (6)
src/libexpr/include/nix/expr/eval.hh (1)
1125-1147: Keepexecutoras the final data member to preserve destruction order.
This placesrootProvenanceafterexecutor, which violates the “keep this last” guarantee. Please moverootProvenancebeforeexecutor(or moveexecutorback to the end).src/libfetchers/tarball.cc (2)
18-34: Registerfetchurlprovenance for deserialization.
Without aProvenance::Registerfor"fetchurl", deserialization will fall back toUnknownProvenance, losing typed data.🧩 Suggested registration (example)
+static Provenance::Register registerFetchurlProvenance( + "fetchurl", + [](nlohmann::json json) { + auto & obj = getObject(json); + return make_ref<FetchurlProvenance>(getString(valueAt(obj, "url"))); + });
107-108: Avoid persisting credentials or signed tokens in provenance URLs.
Storing the raw URL in provenance risks leaking userinfo or signed query params into.narinfo/metadata. Please redact/sanitize before storing.🛡️ Example guard (pseudo)
- info.provenance = std::make_shared<FetchurlProvenance>(url); + info.provenance = std::make_shared<FetchurlProvenance>(redactSensitiveUrl(url));src/libutil/experimental-features.cc (1)
323-330: Suppress tracking links whentrackingUrlis empty.
documentExperimentalFeatures()always emits a tracking link; empty URL yields a broken link for provenance.🧾 Suggested guard
- docOss << fmt( - "\nRefer to [%1% tracking issue](%2%) for feature tracking.", xpFeature.name, xpFeature.trackingUrl); + if (!xpFeature.trackingUrl.empty()) { + docOss << fmt( + "\nRefer to [%1% tracking issue](%2%) for feature tracking.", xpFeature.name, xpFeature.trackingUrl); + }src/libstore/include/nix/store/path-info.hh (1)
127-131: Add<memory>include forstd::shared_ptr(duplicate).This header now stores
std::shared_ptr, so it should include<memory>directly to avoid reliance on transitive includes.🔧 Suggested fix
`#include` <string> `#include` <optional> +#include <memory>src/libfetchers/filtering-source-accessor.cc (1)
71-76: Preserve access checks and subpath provenance (Line 71-76).This override skips
checkAccess()and dropsSubpathProvenancewrapping for non-root paths, so callers can obtain root provenance for any subpath. Consider mirroringSourceAccessor::getProvenance.🧩 Proposed fix
std::shared_ptr<const Provenance> FilteringSourceAccessor::getProvenance(const CanonPath & path) { + checkAccess(path); if (provenance) - return provenance; + return path.isRoot() ? provenance : std::make_shared<SubpathProvenance>(provenance, path); return next->getProvenance(prefix / path); }If needed, add the
SubpathProvenanceinclude to this file.
🧹 Nitpick comments (3)
src/libstore/path-info.cc (1)
219-220: Consider limiting provenance to JSON format V2 (Line 219-220, Line 296-300).To avoid surprising strict V1 consumers, consider gating provenance emission and parsing to
PathInfoJsonFormat::V2only.♻️ Suggested adjustment
- if (experimentalFeatureSettings.isEnabled(Xp::Provenance)) + if (experimentalFeatureSettings.isEnabled(Xp::Provenance) && format == PathInfoJsonFormat::V2) jsonObject["provenance"] = provenance ? provenance->to_json() : nullptr;- if (experimentalFeatureSettings.isEnabled(Xp::Provenance)) { + if (experimentalFeatureSettings.isEnabled(Xp::Provenance) && format == PathInfoJsonFormat::V2) { auto prov = json.find("provenance"); if (prov != json.end() && !prov->second.is_null()) res.provenance = Provenance::from_json(prov->second); }Also applies to: 296-300
src/libstore/provenance.cc (1)
6-25: Inconsistent handling of optionalnextfield in JSON serialization.
DerivationProvenance::to_json()always emits thenextfield (withnullif absent), whileCopiedProvenance::to_json()only emitsnextwhen present. This asymmetry may cause issues with JSON comparison, diffing, or consumers that expect consistent structure.Consider aligning the behavior—either always emit
next(with null) or conditionally emit it in both classes.Option A: Always emit `next` in CopiedProvenance (matching DerivationProvenance)
nlohmann::json CopiedProvenance::to_json() const { - nlohmann::json j{ + return nlohmann::json{ {"type", "copied"}, {"from", from}, + {"next", next ? next->to_json() : nlohmann::json(nullptr)}, }; - if (next) - j["next"] = next->to_json(); - return j; }Option B: Conditionally emit `next` in DerivationProvenance (matching CopiedProvenance)
nlohmann::json DerivationProvenance::to_json() const { - return nlohmann::json{ + nlohmann::json j{ {"type", "derivation"}, {"drv", drvPath.to_string()}, {"output", output}, - {"next", next ? next->to_json() : nlohmann::json(nullptr)}, }; + if (next) + j["next"] = next->to_json(); + return j; }src/libstore/build/substitution-goal.cc (1)
72-72: Consider returning provenance for AlreadyValid paths.
Right now,AlreadyValidreportsnullptreven though provenance may exist in the store. If consumers rely on it, consider fetching it here.💡 Optional improvement
- if (!repair && worker.store.isValidPath(storePath)) { - co_return doneSuccess(BuildResult::Success::AlreadyValid, nullptr); - } + if (!repair && worker.store.isValidPath(storePath)) { + auto info = worker.store.maybeQueryPathInfo(storePath); + co_return doneSuccess(BuildResult::Success::AlreadyValid, info ? info->provenance : nullptr); + }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/libutil/include/nix/util/provenance.hh`:
- Around line 12-23: The Provenance polymorphic base class lacks a virtual
destructor; add a public virtual destructor to Provenance (e.g., declare a
virtual ~Provenance() = default) so derived instances (referenced by
std::shared_ptr<const Provenance> and used by functions like from_json_str,
from_json_str_optional, from_json, to_json_str, and the pure virtual to_json)
are safely destroyed when deleted via a base pointer.
♻️ Duplicate comments (1)
src/libutil/include/nix/util/provenance.hh (1)
3-8: Add missing standard library headers.This header uses
std::map,std::string,std::string_view, andstd::shared_ptrbut relies on transitive includes. Add explicit headers for better portability.`#include` "nix/util/ref.hh" `#include` "nix/util/canon-path.hh" `#include` <functional> +#include <map> +#include <memory> +#include <string> +#include <string_view> `#include` <nlohmann/json_fwd.hpp>
🧹 Nitpick comments (1)
src/libutil/include/nix/util/provenance.hh (1)
34-40: Consider logging or asserting on duplicate type registration.
insert_or_assignsilently overwrites existing registrations. If two modules accidentally register the same type name, debugging would be difficult. Consider usinginsertand checking the return value, or at least logging when a type is overwritten.
src/libcmd/installable-flake.cc
Outdated
|
|
||
| auto attrPath = attr->getAttrPathStr(); | ||
|
|
||
| state->setRootProvenance(makeProvenance(attrPath)); |
There was a problem hiding this comment.
Rather than having to call this, is there any way we could maybe make this automatic?
(Mostly thinking about protecting from developer error -- if I have to write code here for whatever reason, and add a new path where I should have called setRootProvenance but forgot to, that's probably not great. But if it's integrated into the construction of e.g. attrPath, that means we can't forget it because it'll always happen)
|
(Also if you could go through the coderabbit comments and see which ones are real things to be resolved or just LLM hallucinations and close them accordingly, that would be great) |
Example of a substitution event:
{
"action": "result",
"id": 0,
"payload": {
"builtOutputs": {},
"path": "3bb116cnl86svn2lgc41a3i4a9qblgsf-libtool-2.4.7",
"provenance": {
"from": "https://cache.nixos.org",
"type": "copied"
},
"startTime": 0,
"status": "Substituted",
"stopTime": 0,
"success": true,
"timesBuilt": 0
},
"type": 110
}
Example of a derivation event:
{
"action": "result",
"id": 3381333262860569,
"payload": {
"builtOutputs": {
"out": {
"dependentRealisations": {},
"id": "sha256:deb37b0f322203d852a27010200f08e2dd739cb02b51d77999bd7f3162cdfe39!out",
"outPath": "6b9w3gdjnbdvi50c0h0b9xg91hq6aryl-patchelf-0.18.0",
"signatures": []
}
},
"cpuSystem": 2118013,
"cpuUser": 11471586,
"path": {
"drvPath": "fm8zrgh4dazysyz3imcva658h0iv34k0-patchelf-0.18.0.drv",
"outputs": [
"*"
]
},
"provenance": {
"flakeOutput": "packages.x86_64-linux.default",
"next": {
"attrs": {
"dirtyRev": "bb2f1eb3c1e4dc9c4523642a3e39d55806fc9a81-dirty",
"dirtyShortRev": "bb2f1eb-dirty",
"lastModified": 1768573749,
"type": "git",
"url": "file:///home/eelco/Dev/patchelf"
},
"type": "tree"
},
"type": "flake"
},
"startTime": 1768993105,
"status": "Built",
"stopTime": 1768993120,
"success": true,
"timesBuilt": 1
},
"type": 110
}
edolstra
force-pushed
the
provenance-detsys
branch
from
392a12a to
e7453a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/libstore/provenance.cc`:
- Around line 27-33: Add a Provenance::Register for the "derivation" type
mirroring registerCopiedProvenance so Provenance::from_json returns a
DerivationProvenance instead of UnknownProvenance; create a new static register
(e.g. registerDerivationProvenance) that uses getObject(json), reads any "next"
via optionalValueAt(obj, "next") and deserializes it with Provenance::from_json,
and extracts required fields with valueAt/getString to construct and return
make_ref<DerivationProvenance>(...) using the same pattern as
registerCopiedProvenance.
♻️ Duplicate comments (12)
src/libexpr/include/nix/expr/eval.hh (1)
1140-1153: Member ordering violates destruction order requirement.The comment on lines 1133-1138 explicitly states that
executormust be the last data member to ensure it's destroyed first. However,rootProvenanceis now declared afterexecutor(line 1144), makingexecutorno longer the last member.In C++, members are destroyed in reverse declaration order. With this ordering,
rootProvenancewill be destroyed beforeexecutor, meaningexecutoris destroyed second—not first as required.Move
rootProvenancebeforeexecutorto preserve the intended destruction order.src/libstore/include/nix/store/path-info.hh (1)
127-132: Include<memory>forstd::shared_ptrin this public header.This header now exposes
std::shared_ptr, so it should include<memory>directly to avoid relying on transitive includes.🔧 Proposed fix
`#include` <string> `#include` <optional> +#include <memory>src/libutil/experimental-features.cc (1)
342-349: Avoid empty tracking URLs to prevent broken documentation links.
documentExperimentalFeatures()always emits a tracking link, so an empty URL becomes a broken link. Consider adding a real URL or suppressing link emission when empty.🧾 Suggested guard in documentation generation
- docOss << fmt( - "\nRefer to [%1% tracking issue](%2%) for feature tracking.", xpFeature.name, xpFeature.trackingUrl); + if (!xpFeature.trackingUrl.empty()) { + docOss << fmt( + "\nRefer to [%1% tracking issue](%2%) for feature tracking.", xpFeature.name, xpFeature.trackingUrl); + }src/libstore/include/nix/store/provenance.hh (1)
9-58: Verify JSON registration for the new provenance variants.Please confirm
DerivationProvenanceandCopiedProvenanceare registered for JSON deserialization; otherwiseProvenance::from_json*()will fail on these types.#!/bin/bash # Locate provenance registrations for derivation/copied variants fd -a "provenance.cc" --type f -x rg -n "Provenance::Register|DerivationProvenance|CopiedProvenance" -C2 {}src/libstore/include/nix/store/legacy-ssh-store.hh (1)
76-79:isUsefulProvenance()returnstruebut ServeProto does not transmit provenance.This store uses ServeProto, which lacks provenance support in its serialization. Returning
truemisleads callers into attaching provenance metadata that will be silently dropped. Either returnfalseto accurately reflect protocol capabilities, or extend ServeProto to include provenance support.src/libutil/include/nix/util/provenance.hh (2)
3-8: Add explicit standard library headers for used types.The header uses
std::map,std::string,std::string_view, andstd::shared_ptrbut relies on transitive includes fromref.hhandcanon-path.hh. Add explicit headers for better portability and self-containment.♻️ Suggested includes
`#include` "nix/util/ref.hh" `#include` "nix/util/canon-path.hh" `#include` <functional> +#include <map> +#include <memory> +#include <string> +#include <string_view> `#include` <nlohmann/json_fwd.hpp>
12-23: Add virtual destructor to polymorphic base class.
Provenanceis used polymorphically (stored viastd::shared_ptr<const Provenance>inSubpathProvenance::nextand throughout the codebase). Deleting a derived object through a base pointer without a virtual destructor is undefined behavior.🐛 Proposed fix
struct Provenance { + virtual ~Provenance() = default; + static ref<const Provenance> from_json_str(std::string_view);src/libflake/include/nix/flake/flake.hh (1)
98-101: Provenance member addition looks good.The
std::shared_ptr<const Provenance>member with documentation is well-structured. The explicit destructor declaration on line 123 ensures the complete type will be available where~Flake()is defined.As noted in a previous review, ensure
flake.ccincludes the fullprovenance.hhheader whereFlake::~Flake()is defined, sincestd::shared_ptrdestruction requires the complete type.src/libfetchers/tarball.cc (2)
18-34: RegisterFetchurlProvenancefor deserialization.The
FetchurlProvenancetype definesto_json()but lacks a correspondingProvenance::Registerinstantiation. Without registration, deserializing provenance JSON with"type": "fetchurl"will fall back toUnknownProvenance, losing the typed information.
107-107: Consider sanitizing URL before storing in provenance.Storing the raw URL in provenance can persist credentials (userinfo) or signed query parameters into store metadata and
.narinfofiles. Consider redacting sensitive URL components before creatingFetchurlProvenance.src/libfetchers/filtering-source-accessor.cc (1)
71-76: Enforce access checks and subpath provenance wrapping.This override skips
checkAccess()and returns root provenance for subpaths, which drops subpath context and can expose provenance for disallowed paths. IfSubpathProvenanceisn’t already visible here, add the appropriate provenance header.🛠️ Suggested fix
std::shared_ptr<const Provenance> FilteringSourceAccessor::getProvenance(const CanonPath & path) { + checkAccess(path); if (provenance) - return provenance; + return path.isRoot() ? provenance : std::make_shared<SubpathProvenance>(provenance, path); return next->getProvenance(prefix / path); }src/libstore/dummy-store.cc (1)
235-243: Provenance parameter is currently dropped inaddToStoreFromDump.
provenanceis accepted but never assigned to the generatedValidPathInfo, so dummy-store paths lose provenance.🛠️ Suggested fix
info.narSize = narHash.second.value(); + info.provenance = provenance;Also applies to: 286-291
🧹 Nitpick comments (2)
src/libstore/provenance.cc (1)
6-14: DerivationProvenance always includesnextas null, unlike other types.
DerivationProvenance::to_json()always includes the"next"key (withnullptrwhen absent), whileSubpathProvenanceandCopiedProvenanceconditionally omit it. Consider aligning the serialization pattern for consistency.♻️ Optional: align with conditional pattern
nlohmann::json DerivationProvenance::to_json() const { - return nlohmann::json{ + nlohmann::json j{ {"type", "derivation"}, {"drv", drvPath.to_string()}, {"output", output}, - {"next", next ? next->to_json() : nlohmann::json(nullptr)}, }; + if (next) + j["next"] = next->to_json(); + return j; }src/libstore/build-result.cc (1)
103-165: Guard provenance serialization forAlreadyValidresults.When
AlreadyValidorResolvesToAlreadyValidstatus is present, provenance should not be serialized since the existing outputs may have been built with different provenance than the current derivation. Currently,to_json()will emit provenance if present regardless of status, creating a risk of serializing misleading provenance if accidentally set. Add defensive guards during serialization and deserialization to prevent this.♻️ Suggested guards
- if (success.provenance) + if (success.provenance + && success.status != BuildResult::Success::AlreadyValid + && success.status != BuildResult::Success::ResolvesToAlreadyValid) res["provenance"] = success.provenance->to_json();- s.provenance = provenanceFromJson(optionalValueAt(json, "provenance")); + s.provenance = provenanceFromJson(optionalValueAt(json, "provenance")); + if (s.status == BuildResult::Success::AlreadyValid + || s.status == BuildResult::Success::ResolvesToAlreadyValid) + s.provenance.reset();
Makes it clearer that this store path is not a derivation itself, but the result of building a derivation.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/libstore/provenance.cc`:
- Around line 27-33: Add a Provenance::Register for the "build" type so
Provenance::from_json() can deserialize BuildProvenance; mirror the existing
registerCopiedProvenance pattern: register a Provenance::Register("build",
[](nlohmann::json json){ ... }) that calls getObject(json), extracts optional
next via optionalValueAt and converts it with Provenance::from_json, and
constructs and returns make_ref<BuildProvenance>(...) passing the string/fields
read from valueAt/getString for the keys BuildProvenance expects (match the
BuildProvenance constructor parameters). Ensure the lambda uses the same helper
functions (getObject, optionalValueAt, valueAt, getString) and returns a shared
ref to BuildProvenance.
🧹 Nitpick comments (1)
src/libstore/provenance.cc (1)
6-14: Inconsistent handling ofnextfield compared toCopiedProvenance.
BuildProvenance::to_json()always emits"next"(asnullif absent), whileCopiedProvenance::to_json()omits the field entirely whennextis null. Consider aligning the pattern for consistency—either both emitnullor both omit the field.♻️ Suggestion to align with CopiedProvenance pattern
nlohmann::json BuildProvenance::to_json() const { - return nlohmann::json{ + nlohmann::json j{ {"type", "build"}, {"drv", drvPath.to_string()}, {"output", output}, - {"next", next ? next->to_json() : nlohmann::json(nullptr)}, }; + if (next) + j["next"] = next->to_json(); + return j; }
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/libstore/store-api.cc`:
- Around line 875-886: Replace the use of srcStore.config.getHumanReadableURI()
in addCopiedProvenance with the direct stable renderer: call
srcStore.config.getReference().render(/*withParams=*/false) when constructing
the CopiedProvenance so the persisted provenance uses the same stable identifier
pattern as other stores (e.g., binary-cache-store.cc) and avoids relying on
getHumanReadableURI() indirection.
♻️ Duplicate comments (2)
src/libstore/include/nix/store/legacy-ssh-store.hh (1)
76-79: Verify if returningtrueis intentional given ServeProto limitations.A past review raised concerns that ServeProto doesn't support provenance transmission in its ReadConn/WriteConn structures. The method was renamed from
isUsefulProvenance()toincludeInProvenance(), but the return value remainedtrue.If
includeInProvenance()means "should this store be recorded in provenance when paths are copied FROM it", then returningtrueis appropriate. If it means "can this store persist/transmit provenance data", then it may be misleading.src/libfetchers/tarball.cc (1)
12-34: Confirmfetchurlprovenance is registered for deserialization.I don’t see the
Provenance::Registerhookup in this file; if it’s missing elsewhere,"type":"fetchurl"will deserialize asUnknownProvenance. Please verify it exists (or add it in a fetchers provenance registry).#!/bin/bash # Verify fetchurl provenance registration exists somewhere in the repo. rg -nP 'Provenance::Register|register.*Provenance' src/libfetchers src/libutil src/libstore rg -nP '"fetchurl"' src/libfetchers src/libutil src/libstore
This allows provenance to be propagated correctly to worker threads.
edolstra
force-pushed
the
provenance-detsys
branch
from
dcc7f77 to
bdc94b2
Compare
2 participants
Motivation
This is an updated version of upstream NixOS#11749.
Nix historically has been bad at being able to answer the question "where did this store path come from", i.e. to provide traceability from a store path back to the Nix expression from which is was built. Nix tracks the "deriver" of a store path (the
.drvfile that built it) but that's pretty useless in practice, since it doesn't link back to the Nix expressions.So this PR adds a "provenance" field (a JSON object) to the
ValidPathstable and to.narinfofiles that describes where the store path came from and how it can be reproduced.There are currently the following types of provenance:
copied: Records that the store path was copied or substituted from another store (typically a binary cache). Its "from" field is the URL of the origin store. Its "provenance" field propagates the provenance of the store path on the origin store.build: Records that the store path was produced by building a derivation. This is equivalent for the "deriver" field, but it has a nested "provenance" field that records how the .drv file was created.tree: The store path is the result of a call tofetchTree(e.g. it's a flake or flake input). It includes the fetcher attributes.subpath: The store path was created by copying the subpath of some other provenance (e.g./foo/builder.shfrom atree) .flake: Records that the store path was created during the evaluation of a flake output.fetchurl: The store path is the result of abuiltins.fetchurlcall.Context
Summary by CodeRabbit