Add --provenance attestation sidecar - #25
Conversation
--provenance writes <pkg>.provenance.json next to the package recording the tool version, build time, name/version/identifier, package path and sha256, a deterministic input digest (sorted hash of payload + scripts + build-info), and the git commit/remote when the project is a repo. Remote URLs are stripped of user:pass@ credentials before recording. Threaded through PackageBuildOptions so the GUI can adopt it. git is added to ToolPaths and probed via ProcessRunning (absent repo -> null). Tests: ProvenanceTests (credential stripping incl. scp-style passthrough; git capture; deterministic digest; digest changes with inputs; snake_case JSON round-trip). verify-loop.sh builds with --provenance and validates the sidecar keys and that sha256 matches the real package.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesPackage provenance
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant CLI
participant PackageBuildCoordinator
participant ProvenanceBuilder
participant PackageFile
participant ProvenanceJSON
CLI->>PackageBuildCoordinator: enable provenance
PackageBuildCoordinator->>PackageFile: build package
PackageBuildCoordinator->>ProvenanceBuilder: build provenance
ProvenanceBuilder->>PackageFile: compute SHA-256
ProvenanceBuilder->>ProvenanceJSON: serialize metadata
PackageBuildCoordinator->>ProvenanceJSON: atomically write sidecar
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 57 minutes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
swiftpkgTests/ProvenanceTests.swift (1)
28-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test for payload-free provenance generation.
Both tests populate
payload/, so theinputDigestcode path that skips a missingpayload/scriptsdirectory (Provenance.swift lines 69-78) is untested here. As per coding guidelines, "payload-free packages" is explicitly called out as a scenario requiring a targeted compatibility test when new behavior touches it.🤖 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 `@swiftpkgTests/ProvenanceTests.swift` around lines 28 - 86, Add a targeted test for payload-free provenance generation alongside buildsProvenance and digestChangesWithInputs, creating a project with build metadata and output but no payload or scripts directory. Build provenance through ProvenanceBuilder and assert it succeeds with a valid deterministic inputDigest, covering the missing-directory handling in the provenance input collection path.Source: Coding guidelines
swiftpkg/Provenance.swift (2)
67-92: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
inputDigestloads each file fully into memory;provenanceSHA256streams in chunks.Two hashing helpers in the same file use different strategies:
inputDigestreads each file entirely viaData(contentsOf:)per iteration, whileprovenanceSHA256(ofFileAt:)streams in 1 MB chunks viaFileHandle. For large payload files this creates avoidable memory spikes and duplicated hex-encoding logic (hasher.finalize().map { String(format: "%02x", $0) }.joined()appears twice). Consider reusing the streaming approach/helper for both.♻️ Suggested consolidation
- for entry in entries { - hasher.update(data: Data(entry.path.utf8)) - hasher.update(data: Data([0])) - hasher.update(data: try Data(contentsOf: entry.url)) - } - return hasher.finalize().map { String(format: "%02x", $0) }.joined() + for entry in entries { + hasher.update(data: Data(entry.path.utf8)) + hasher.update(data: Data([0])) + try streamHash(of: entry.url, into: &hasher) + } + return hexDigest(hasher.finalize())Add a shared
streamHash(of:into:)helper (chunkedFileHandlereads) and a sharedhexDigest(_:)helper, and reuse both fromprovenanceSHA256(ofFileAt:).🤖 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 `@swiftpkg/Provenance.swift` around lines 67 - 92, Update inputDigest to hash each entry through a shared chunked FileHandle streaming helper instead of Data(contentsOf:), preserving the existing path-and-null-byte prefix ordering. Extract the repeated SHA256 hex conversion into a shared hexDigest helper, and reuse both helpers from inputDigest and provenanceSHA256(ofFileAt:).
30-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStatic analysis: prefer failable
String(bytes:encoding:).SwiftLint flags
String(decoding: try encoder.encode(self), as: UTF8.self). Functionally fine here sinceJSONEncoderoutput is guaranteed valid UTF-8, so this is a low-value stylistic nit, but addressing it would keep lint clean.🤖 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 `@swiftpkg/Provenance.swift` around lines 30 - 34, Update jsonString() to construct the encoded JSON using failable String(bytes:encoding:) instead of String(decoding:as:), while preserving the existing JSONEncoder configuration and throwing behavior for encoding failures. Handle the optional string result appropriately so the method still returns a String.Source: Linters/SAST tools
🤖 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 `@swiftpkg/Provenance.swift`:
- Around line 67-92: Update inputDigest to hash each entry through a shared
chunked FileHandle streaming helper instead of Data(contentsOf:), preserving the
existing path-and-null-byte prefix ordering. Extract the repeated SHA256 hex
conversion into a shared hexDigest helper, and reuse both helpers from
inputDigest and provenanceSHA256(ofFileAt:).
- Around line 30-34: Update jsonString() to construct the encoded JSON using
failable String(bytes:encoding:) instead of String(decoding:as:), while
preserving the existing JSONEncoder configuration and throwing behavior for
encoding failures. Handle the optional string result appropriately so the method
still returns a String.
In `@swiftpkgTests/ProvenanceTests.swift`:
- Around line 28-86: Add a targeted test for payload-free provenance generation
alongside buildsProvenance and digestChangesWithInputs, creating a project with
build metadata and output but no payload or scripts directory. Build provenance
through ProvenanceBuilder and assert it succeeds with a valid deterministic
inputDigest, covering the missing-directory handling in the provenance input
collection path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d35bbd2-885e-453b-898e-bdbc66b5ff2a
📒 Files selected for processing (7)
scripts/verify-loop.shswiftpkg/PackageBuildOptions.swiftswiftpkg/PackageBuilder.swiftswiftpkg/Provenance.swiftswiftpkg/Support.swiftswiftpkgCLI/CLI.swiftswiftpkgTests/ProvenanceTests.swift
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
swiftpkgTests/ProvenanceTests.swift (1)
5-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer the shared
RecordingRunnerover a bespokeGitRunnertest double.As per path instructions,
swiftpkgTests/**/*.swift: "Keep unit tests hermetic: useTemporaryDirectoryandRecordingRunnerfromTestSupport.swiftinstead of real subprocesses when testing command construction." This file defines its ownProcessRunningstub instead of reusingRecordingRunner, duplicating existing test infrastructure.🤖 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 `@swiftpkgTests/ProvenanceTests.swift` around lines 5 - 16, Replace the bespoke GitRunner test double with the shared RecordingRunner from TestSupport.swift in the affected provenance tests. Configure its recorded command outputs to provide the commit and remote values currently returned by GitRunner, while preserving the tests’ existing command-construction assertions and hermetic behavior.Source: Path instructions
scripts/verify-loop.sh (1)
74-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider also exercising
--provenanceagainst a payload-free/empty-payload project.As per path instructions,
scripts/verify-loop.shmust exercise "payload-free and empty-payload projects" among other scenarios. The new provenance block only tests a project with a payload; adding a quick payload-free variant would confirminputDigestbehaves correctly when the "payload"/"scripts" directories don't exist.🤖 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 `@scripts/verify-loop.sh` around lines 74 - 91, Extend the provenance coverage in the verification flow to include a payload-free or empty-payload project where neither payload nor scripts directories exist. Invoke the existing --provenance path for that project and validate the generated provenance, especially that input_digest is produced correctly without payload content, while preserving the current payload-backed assertions.Source: Path instructions
🤖 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 `@swiftpkg/Provenance.swift`:
- Around line 65-98: Update inputDigest(for:) to include symlink entries instead
of filtering them out, hashing a deterministic representation of each link
target alongside its relative path. Also read and include POSIX permission bits
for every included filesystem entry, especially scripts, in the digest input.
Preserve sorted project-relative ordering and the existing content hashing for
regular files.
---
Nitpick comments:
In `@scripts/verify-loop.sh`:
- Around line 74-91: Extend the provenance coverage in the verification flow to
include a payload-free or empty-payload project where neither payload nor
scripts directories exist. Invoke the existing --provenance path for that
project and validate the generated provenance, especially that input_digest is
produced correctly without payload content, while preserving the current
payload-backed assertions.
In `@swiftpkgTests/ProvenanceTests.swift`:
- Around line 5-16: Replace the bespoke GitRunner test double with the shared
RecordingRunner from TestSupport.swift in the affected provenance tests.
Configure its recorded command outputs to provide the commit and remote values
currently returned by GitRunner, while preserving the tests’ existing
command-construction assertions and hermetic behavior.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 95ada447-5d5b-4b5b-b2a5-9ce6a504cb17
📒 Files selected for processing (7)
scripts/verify-loop.shswiftpkg/PackageBuildOptions.swiftswiftpkg/PackageBuilder.swiftswiftpkg/Provenance.swiftswiftpkg/Support.swiftswiftpkgCLI/CLI.swiftswiftpkgTests/ProvenanceTests.swift
The input digest skipped symlinks entirely and never hashed POSIX permission bits, so a changed framework symlink target or a toggled executable bit produced an identical input_digest — defeating the attestation. Symlinks now contribute their destination and every entry contributes its mode.
|
Addressed in the latest commit. The enumerator now includes symlinks (hashing their destination rather than skipping them), and every entry's POSIX mode bits are folded into the digest, so a toggled executable bit or a re-pointed |
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)
swiftpkgTests/ProvenanceTests.swift (1)
51-53: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert digest values, not only their lengths.
count == 64allows any 64-character string, including an incorrect or non-hex digest, to pass. Compare the package digest against an independently computed expected SHA-256 forPKGDATA, and validate the input digest against a deterministic golden value or independent reference implementation.🤖 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 `@swiftpkgTests/ProvenanceTests.swift` around lines 51 - 53, Update the assertions in ProvenanceTests around provenance to compare sha256 and inputDigest against independently computed deterministic SHA-256 golden values for PKGDATA, rather than checking only that each string has length 64. Preserve the existing provenance identifier assertion.
🤖 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 `@swiftpkgTests/ProvenanceTests.swift`:
- Around line 51-53: Update the assertions in ProvenanceTests around provenance
to compare sha256 and inputDigest against independently computed deterministic
SHA-256 golden values for PKGDATA, rather than checking only that each string
has length 64. Preserve the existing provenance identifier assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 94cfac87-5e29-49b5-af59-7d91f6547c09
📒 Files selected for processing (2)
swiftpkg/Provenance.swiftswiftpkgTests/ProvenanceTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- swiftpkg/Provenance.swift
2 participants
--provenancewrites a sidecar attestation describing how the package was built (inputs, tool version, resulting digest) for supply-chain traceability.Tests:
ProvenanceTests;verify-loop.shcovers it; full suite green.Part of a 9-PR series splitting a batch of features into small, themed, independently reviewable PRs. Each applies cleanly to
mainon its own; the ordering below only minimizes rebases as they land:Happy to squash, split, or reorder any of these to suit your review preferences.
Summary by CodeRabbit
--provenanceflag to generate a.provenance.jsonsidecar next to built packages.input_digest, and optional source control commit/remote info, with remote URL credential sanitization.input_digestchange detection when payload contents, executable bits, or symlink targets change.--provenance.