Rebrand - #50
Merged
Merged
Conversation
SystemProcessRunner.run called waitUntilExit() before reading stdout and stderr, then drained each pipe sequentially. A child that writes more than the ~64KB pipe buffer to either stream blocks on write(), never exits, and waitUntilExit() hangs indefinitely — reachable via pkgbuild --analyze or a verbose productbuild on a large payload. Drain both pipes concurrently on background queues joined with a DispatchGroup before waiting. Signature and ProcessResult are unchanged. Add SystemProcessRunnerTests exercising the real runner: 1.1MB ordered stdout (would hang the old code), stderr separation with non-zero status, and structured launch-failure errors.
waitForAcceptance only warned and returned false on timeout, so a build whose notarization never reached "Accepted" exited 0 with an un-stapled package — a silent CI failure. Make the timeout throw, matching the existing terminal-status failure path. --skip-notarization and --skip-stapling remain the explicit opt-outs. Make NotarizationService internal so the state machine is testable, and add NotarizationServiceTests: timeout throws (staplingTimeout 0, no sleep) and an Accepted submission still staples. Note: this changes exit behavior on notarization timeout (previously 0). To discuss upstream before merge per swiftpkg's issue-first policy.
Every failure previously exited 255, so CI could not tell a bad project from a failed build from a failed notarization. Add an exitCode to MunkiPkgError and route the CLI through it: 0 success 1 general error 2 project already exists 3 invalid configuration 4 import failed 5 build / subprocess failure 7 notarization failed 64 command-line usage error (EX_USAGE) 6 is reserved for a future dedicated signing-failure class (isolating signing from generic pkgbuild/productbuild failure needs more context and is best decided upstream). Reclassify the project-exists, import, and notarization throw sites accordingly; other throws stay general/build. Add ExitCodeTests covering the mapping, the unknown-error default, and the CLI returning 2 / 1 / 64 end-to-end. Note: changes failure exit codes from 255. Success stays 0; anyone checking != 0 is unaffected. To discuss upstream before merge per the issue-first policy.
--pkg-version lets the version come from a git tag or CI variable instead
of the committed build-info; it is resolved before ${version} substitution
so it flows into the package name too. --output-dir writes the package to a
chosen directory (created if absent) instead of the project's build/ dir.
Both are threaded through PackageBuildOptions so the GUI can adopt them
without new plumbing. PackageConfiguration.withVersion applies the override
ahead of substitutingVersion; PackageProjectLayout takes an optional
output directory.
Tests: VersionOverrideTests (withVersion isolation; load applies the
override to version and ${version} in name; no override preserves
build-info). verify-loop.sh builds with --pkg-version + --output-dir and
asserts the artifact lands in the target dir with the overridden version
and not in build/.
Support ${TIMESTAMP} (yyyy.MM.dd.HHmm), ${DATE} (yyyy.MM.dd), and
${DATETIME} (yyyy.MM.dd.HHmmss) in the build-info version field, resolved
before ${version} substitution so the stamp flows into the package name.
Mirrors munki-pkg. Static versions are unaffected.
DynamicVersion.resolve takes an injectable clock so tests are deterministic;
resolvingDynamicVersion applies it at load time.
Tests: DynamicVersionTests (each token's format at a fixed date, embedded
token, static passthrough, and resolved version feeding ${version} in name).
verify-loop.sh builds a project versioned ${DATE} and asserts the package
filename matches the dated pattern.
The output package path is build/<name> (and build/Dist-<name> for
distribution builds), with name coming from build-info after ${version}
substitution. A name containing a path separator or ".." wrote the
artifact outside build/ — a path traversal driven by untrusted build-info.
Validate the resolved name is a single, safe path component before the
build starts, throwing invalidConfiguration otherwise.
Add PackageNameValidationTests covering unsafe/safe names and an
end-to-end build that must throw before pkgbuild is ever invoked.
Build-info files commonly set signing_info.keychain to
${HOME}/Library/Keychains/signing.keychain. swiftpkg passed that value
to productbuild verbatim, so signing failed with "Could not find
appropriate signing identity ... in keychain at ${HOME}/...". Expand
${HOME} to the user home directory and resolve a leading tilde before
handing the path to productbuild/productsign, matching munki-pkg.
munki-pkg prompts to import the built package into a Munki repo and offers --skip-import to suppress that prompt; CI pipelines pass it routinely. swiftpkg never prompts, so it previously rejected the flag as unknown and any pipeline passing --skip-import failed. Accept it as a documented no-op so those invocations work unchanged.
A project that has build-info but neither a payload folder nor a scripts folder is valid: pkgbuild --nopayload produces a receipt-only package that installs no files but records a receipt Munki conditions can key off. swiftpkg rejected these outright; munki-pkg builds them. The component builder already emits --nopayload with no --scripts, so only the up-front guard needed to go.
Loading a project with a present-but-incomplete notarization_info (e.g. a bare password with no apple_id/team_id/keychain_profile) threw at load time, so even --skip-notarization builds failed. munki-pkg tolerates the incomplete block at load and only errors when notarization is actually attempted. Parse it into a new .invalid(reason:) authentication case; notarize() rejects .invalid, while skipped builds proceed unaffected.
build-info may set name without a .pkg suffix (e.g. MunkiBootstrap).
munki-pkg writes the artifact as <name>.pkg; swiftpkg used the name
verbatim, producing an extensionless file that find '*.pkg' and
munkiimport miss. Normalize the resolved name to end in .pkg after
${version} substitution, matching munki-pkg.
action.yml lets any repo build a swiftpkg project on a macOS runner with `uses: codecarton/swiftpkg@v1` instead of hand-rolling install + invoke. It installs the swiftpkg release (Universal .pkg via `gh release download` + installer), optionally lints, builds with --output-format json (+ optional --verify/--provenance/--pkg-version/--output-dir), and exposes pkg-path, version, and sha256 as step outputs. Adapted from munki-pkg's action to swiftpkg's CLI: no --build/--no-import (swiftpkg builds by default), and the binary is installed from the release package rather than downloaded bare. README documents inputs/outputs. Requires a swiftpkg release carrying the CI flags (PRs for --output-format, --output-dir, --pkg-version, --lint, --verify).
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.
--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.
Builds now return a BuildResult (name, version, identifier, pkg_path, sha256, signed, notarized, stapled) whose booleans reflect what actually happened, not what build-info requested. --output-format json prints it as a machine-readable manifest and reserves stdout for that manifest (human status is suppressed there; warnings/errors still go to stderr). Default text output is unchanged. - PackageBuildCoordinator.buildPackage returns BuildResult (@discardableResult); NotarizationService.notarize reports whether it stapled. - PackageOperationService.buildPackage forwards the result so the GUI can consume it later. - sha256Hex streams the package in 1MB chunks (CryptoKit). Tests: hermetic BuildResultTests (known SHA-256 vector, multi-chunk file, snake_case JSON round-trip); verify-loop.sh now builds with json output and asserts the manifest keys, booleans, pkg_path, and sha256 against the real package. swift test, verify-loop, release build, and the Swiftpkgr Xcode build all pass.
azure-pipelines/swiftpkg-build.yml is the ADO equivalent of action.yml: a steps template another pipeline can include to install swiftpkg, optionally lint, build with --output-format json (+ optional --verify/--provenance/ --pkg-version/--output-dir), and expose pkgPath/version/sha256 as output variables on the 'build' step. Runs on a macOS agent. Adapted from munki-pkg's template to swiftpkg's CLI: installs the release Universal .pkg (asset URL resolved from the GitHub release API since the filename embeds the version) instead of a bare binary, and drops --build/--no-import.
Substitute ${VAR} placeholders in install scripts at build time from a
.env file (auto-detected, or --env-file) merged with SWIFTPKG_* process
variables (.env wins; --no-inherit-env disables the merge). --strict-env
fails the build on any unresolved placeholder; otherwise unresolved names
are warned.
Hardening ported from munki-pkg: 1MB file cap, identifier-shape key
validation (invalid keys skipped with a warning), single-pass replacement
(a substituted value is never re-expanded), and processed scripts written
to a private mode-0700 temp dir that pkgbuild packages instead of the
originals. Values are spliced verbatim and land as plain text in the .pkg,
so this is for build-time config, not secrets (documented in-file).
Note: munki-pkg inherited MUNKIPKG_*; this uses SWIFTPKG_* to match the
tool name — worth confirming upstream.
Tests: EnvLoaderTests (parse/quote/comment, invalid-key skip, oversize
reject, merge precedence + inherit toggle), PlaceholderReplacerTests
(substitute/unresolved, single-pass, verbatim splice), ScriptEnvironmentTests
(0700 perms, substitution, empty-vars no-op). verify-loop.sh builds a
project with a .env and asserts the packaged postinstall was substituted.
--lint loads the build-info and checks it without invoking pkgbuild, for a fast PR/CI pre-check. Errors (empty version, name that isn't a single path component, project with neither payload nor scripts, undecodable build-info) exit non-zero; warnings (non-reverse-DNS identifier, name not ending .pkg, notarization without signing, script missing a shebang or exec bit) are advisory. Findings go to stderr; the exit code is the machine signal. Tests: LinterTests cover clean, error, and warning cases. verify-loop.sh lints a good project (passes) and a bad one (must fail).
… codes The notarization timeout threw MunkiPkgError.message and the notarization plist helper and importer plist read let raw errors propagate, so all three bypassed the failure-class exit-code mapping. Classify them as notarizationFailed and importFailed so the CLI reports the right exit code.
Assert that a project with neither payload nor scripts invokes pkgbuild with --nopayload and no --root, using TemporaryDirectory and RecordingRunner per the repo's unit-test conventions.
--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.
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.
…ories The reverse-DNS check only tested for a dot, so ".example", "com..example", and "com.example." passed; it now requires at least two non-empty dot-separated components. lintScripts used fileExists, which also matches a directory, letting a preinstall/postinstall directory skip the shebang check and pass the executable-bit check; it now errors on a directory. Added regression tests plus a scripts-only compatibility case.
Notarization now always polls for acceptance and returns accepted and stapled independently, so the manifest's notarized/stapled flags reflect what actually happened rather than what was requested (a timeout no longer reports notarized:true). JSON quieting now applies only to builds, so commands like --create --output-format json keep their status output. The verify-loop manifest gate replaces assert (stripped by python3 -O) with explicit checks that assert concrete name/version/identifier values, marks the payload fixture executable, and the large-file digest test now asserts the known hash.
…outputs Both the GitHub action and the Azure template now verify the downloaded swiftpkg installer's Developer ID signature and notarization (pkgutil --check-signature + spctl) before running it as root, and note that pinning the version is preferred over 'latest'. The action downloads into a fresh directory and requires exactly one matching asset. The Azure template selects the asset inside jq instead of piping to head (which could SIGPIPE under pipefail), extracts outputs with jq -er so a null fails the step, and strips CR/LF from output values so they can't inject a second logging command. Documented --provenance in the README.
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.
The non-script branch copied with try?, discarding errors, while substituted scripts write with a throwing call. A failed copy left the override scripts directory silently incomplete, so pkgbuild could produce a package missing files with no error. Propagate the copy error instead.
…ness Process and exit-code correctness: pipe drain, notarization failure, distinct exit codes
Version resolution: --pkg-version/--output-dir overrides and dynamic version tokens
munki-pkg drop-in compatibility: name handling, keychain paths, --skip-import, receipt-only, notarization deferral
${VAR} is ambiguous by construction: it is both swiftpkg's build-time
substitution syntax and ordinary shell expansion. The scanner reported every
unsubstituted ${...} it saw, so a helper like
plist_set() { local key="$1" type="$2" val="$3"; ... }
warned about key, type and val on every build — names the script declares and
resolves itself at install time.
Subtract the names a script assigns (plain and declarator-prefixed assignments,
bare declarations, for-loop variables, read targets) from what gets reported. A
build variable that is genuinely missing is never assigned in the script, so it
still warns, and --strict-env still fails on it. Substitution behaviour is
untouched; only reporting narrows.
"Applied 2 build variable(s) to install scripts" counted the entries in the
merged variable map. It said the same thing whether both names were substituted
or neither appeared in any script, so a typo on either side -- a placeholder
spelled one way and a .env key spelled another -- reported success.
Track the names actually replaced. PlaceholderReplacer.Result gains
`substituted` alongside `unresolved`, and ScriptEnvironment.process returns an
Outcome carrying both per script, so the message can name how many of the
loaded variables reached a script and how many scripts they reached:
Applied 1 of 3 build variable(s) to 1 install script(s)
When nothing matched, say that outright rather than report an application that
did not happen.
Every subprocess call routed through runSuccessfully already appends the tool's
stderr to the failure message. The notarytool plist path did not: it discarded
the output and reported a bare "Notarization upload failed."
That cost a real investigation. A CI build failed with only that line, when
notarytool had said exactly what was wrong:
Error: No Keychain password item found for profile: notarization_credentials
Run 'notarytool store-credentials' to create another credential profile.
Hoist the logic into ProcessResult.failureDetail(fallback:) and use it in both
places, so a self-describing configuration error stays self-describing. stdout is
now a fallback for tools that report failures there; since it only applies when
stderr is empty, no existing message loses detail.
Both templates downloaded 'swiftpkg-*-universal.pkg'. No release has ever
published that asset. A release ships swiftpkg-<version>-cli.pkg,
swiftpkg-<version>-combined.pkg, SHA256SUMS, a universal .tar.gz and the
Swiftpkgr zip, so `gh release download` answered "no assets match the file
pattern" and the install step could never have succeeded. Ask for the CLI
package, which is what CI needs.
With the right asset selected, harden what happens to it. It is installed as
root, so three checks now stand between the download and `sudo installer`, each
covering what the others cannot:
- The caller's own swiftpkg-sha256, when set. GitHub release assets can be
replaced without moving the tag, so this is the only check that pins the
bytes; a build that must be reproducible should set it.
- SHA256SUMS from the release. It ships alongside the asset, so it moves with
a replaced release -- it catches a truncated or corrupted download, not a
substituted one.
- Developer ID Team ID plus spctl. spctl establishes that Apple notarized the
package; the Team ID establishes who signed it, which notarization alone
does not. Forging this requires the publisher's certificate.
Default swiftpkg-version to a pinned tag instead of 'latest', so a build does
not silently change when the next release ships. 'latest' still works for anyone
who wants it.
Verified against the real v0.3.1 release on macOS: the install step runs green
end to end (with sudo stubbed), and fails closed on a mismatched checksum and on
an unexpected Team ID.
warnsOnBadScript kept the default payload, so it proved the script findings appear but not that they appear alone. A payload-free project with scripts is supported, and nothing asserted that such a project escapes the "neither a payload nor a non-empty scripts directory" error while still reporting the shebang and permission warnings. Drop the payload from that test and require every finding to be a warning. Also cover the boundary the hasScripts check draws: a scripts directory holding nothing but .DS_Store is not a scripts directory, and a project with only that is still empty.
# Conflicts: # swiftpkg/PackageBuilder.swift
shellOwnedNames only recognises names a script assigns itself, so a script
that reads ${HOME} or ${PATH} — expanded by the shell at install time like
any other environment variable — had them reported as forgotten build
variables. Under --strict-env that failed the build on a correct script.
Add the set of names the install environment is guaranteed to define (the
POSIX/shell set plus the variables macOS installer exports into package
scripts) and subtract it alongside the script-assigned names. The list is
deliberately closed: anything outside it stays reportable, so a genuinely
missing variable beside an environment one is still caught.
Substitution is untouched — only what gets reported narrows — so supplying
one of these in a .env still substitutes it.
--create still stamped com.github.munki.pkg.<name> into every new project, inherited from munki-pkg. The tool is swiftpkg, so the placeholder it writes should be too. Use org.swiftpkg.pkg.<name>, matching the namespace scripts/release.sh already uses for the CLI package and keeping munki-pkg's shape, so it stays an obvious placeholder to replace and still passes the linter's reverse-DNS check. Only the default for newly created projects changes; existing build-info files carry their own identifier and are unaffected. verify-loop.sh asserted the old value, so it moves with it.
…anding Default new projects to an org.swiftpkg identifier
…onment-scope Do not report environment-supplied names as unresolved placeholders
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (41)
📝 WalkthroughWalkthroughChangesBuild contracts and foundation
Package build, verification, and provenance
CLI commands and output handling
GitHub and Azure build integrations
End-to-end verification scenarios
Estimated code review effort: 5 (Critical) | ~90+ minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant Release
participant swiftpkg
participant PackageBuilder
participant Output
CI->>Release: Download pinned CLI release
Release-->>CI: Installer and checksum metadata
CI->>CI: Verify checksum, Team ID, and spctl assessment
CI->>swiftpkg: Run lint and build commands
swiftpkg->>PackageBuilder: Build with configured options
PackageBuilder-->>swiftpkg: BuildResult JSON
swiftpkg-->>Output: Publish package path, version, and SHA-256
Possibly related PRs
Suggested labels: ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Merged
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
2 participants
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.
Summary by CodeRabbit