Add --verify to check the built package against build-info - #24
Conversation
After building, --verify asserts reality matches what build-info declared: pkgutil --check-signature when signing was requested, and an spctl Gatekeeper assessment when notarization was requested. A mismatch fails the build. On an unsigned/un-notarized build it is a no-op. Threaded through PackageBuildOptions so the GUI can adopt it. The notarization branch in buildPackage becomes an if-let so verification still runs when no notarization is configured. Tests: PackageVerifierTests (no checks when nothing declared; signature check runs and passes on status 0, fails on non-zero; notarized runs spctl). verify-loop.sh builds with --verify.
|
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:
📝 WalkthroughWalkthroughThe PR adds a ChangesPackage verification
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 (1)
swiftpkgTests/PackageVerifierTests.swift (1)
5-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
RecordingRunnerfor command assertions.
StatusRunnerduplicates the shared recording test double. UseRecordingRunnerfor command-construction assertions, retaining a minimal result-configurable fake only where failure-status simulation requires 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/PackageVerifierTests.swift` around lines 5 - 16, Replace StatusRunner in command-construction assertions with the shared RecordingRunner test double. Retain a minimal status-configurable fake only in tests that specifically simulate non-success process statuses, and update affected setup and assertions accordingly.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.
Nitpick comments:
In `@swiftpkgTests/PackageVerifierTests.swift`:
- Around line 5-16: Replace StatusRunner in command-construction assertions with
the shared RecordingRunner test double. Retain a minimal status-configurable
fake only in tests that specifically simulate non-success process statuses, and
update affected setup and assertions accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 23630337-21bd-43ee-a739-4339616c24c6
📒 Files selected for processing (7)
scripts/verify-loop.shswiftpkg/PackageBuildOptions.swiftswiftpkg/PackageBuilder.swiftswiftpkg/PackageVerifier.swiftswiftpkg/Support.swiftswiftpkgCLI/CLI.swiftswiftpkgTests/PackageVerifierTests.swift
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/PackageVerifier.swift`:
- Around line 12-27: Extend verify(package:signed:notarized:) to accept the
expected build-info identifier and version, then retrieve the package metadata
and compare both values before verification succeeds. Throw
MunkiPkgError.message with the existing verification-failure pattern when either
value mismatches, and update callers to pass the expected metadata through this
path.
In `@swiftpkgTests/PackageVerifierTests.swift`:
- Around line 5-16: Replace the local StatusRunner test double with the shared
RecordingRunner from TestSupport.swift, configuring its results to represent
successful and failing subprocess calls as needed. Replace the fixed /tmp URL
with TemporaryDirectory, and update the affected tests to use these helpers
while preserving their command-construction assertions.
🪄 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: ac4e1eab-2d96-4d61-a4f3-31e759fd95f9
📒 Files selected for processing (7)
scripts/verify-loop.shswiftpkg/PackageBuildOptions.swiftswiftpkg/PackageBuilder.swiftswiftpkg/PackageVerifier.swiftswiftpkg/Support.swiftswiftpkgCLI/CLI.swiftswiftpkgTests/PackageVerifierTests.swift
--verify now expands the built package's PackageInfo and fails when its identifier or version differ from build-info, so a stale or mismatched artifact can no longer pass verification on signature and Gatekeeper alone. The metadata comparison is factored into a pure function; the verifier tests now use the shared RecordingRunner and TemporaryDirectory helpers.
|
Both addressed in the latest commit: |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/PackageVerifier.swift`:
- Around line 42-46: Update the verification flow around runner.run and the
PackageInfo loading guard so a nonzero pkgutil --expand status, or
unreadable/malformed expansion output, throws an error containing the command
diagnostics instead of returning successfully. Preserve the skip only when
expansion succeeds and the distribution package has no top-level PackageInfo.
- Around line 60-65: Update the verification logic after parser.parse() and
delegate.pkgInfo succeeds to require both “identifier” and “version” attributes
before comparing them with expectedIdentifier and expectedVersion. Return an
appropriate validation error for missing metadata, while preserving the existing
nil/best-effort behavior only when distribution metadata cannot be extracted or
parsed.
In `@swiftpkgTests/PackageVerifierTests.swift`:
- Around line 17-19: Update the assertions in the verifier test to require that
runner.calls is non-empty before checking that every call contains “--expand”.
Preserve the existing assertions that signature checking and spctl are not
invoked, and ensure the test fails when verify performs no tool calls.
🪄 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: e827be45-f1b1-4fb8-b1d9-e2f7751beee5
📒 Files selected for processing (3)
swiftpkg/PackageBuilder.swiftswiftpkg/PackageVerifier.swiftswiftpkgTests/PackageVerifierTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- swiftpkg/PackageBuilder.swift
Address follow-up review of the metadata check: a non-zero pkgutil --expand now fails verification instead of silently skipping, and a PackageInfo that parses but omits identifier or version is rejected rather than passed. Tests assert the expansion ran and cover both new failure paths; RecordingRunner gains an optional per-call result provider.
|
Follow-up review addressed: a non-zero |
2 participants
--verifyre-reads a built package and checks it matches build-info (identifier, version, signing / notarization state) — a cheap post-build gate for CI.Tests:
PackageVerifierTests;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
--verifyflag to validate a built package after creation.--verifybehavior.