refactor(release): one notarization path, one scratch-directory helper, one plugin build - #2365
Merged
Merged
Conversation
…r, one plugin build Claude-Session: https://claude.ai/code/session_013MEaba8K1HQcyDNeq5wEFk
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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
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.
Three duplications in the release scripts. The notarization one had leaked real rigor.
Notarization existed three times, at three rigor levels
stapler validatespctl)build-plugin.shcreate-dmg.shbuild-release.shThe app, the artifact users actually download, got the weakest checks. The DMG and the plugins got the best diagnostics withheld: when a submission failed, only the app path fetched the notary log that says why.
scripts/lib/notarize.shis one implementation at the strictest level all three reached between them. All three callers now get every column.The two post-staple checks are not redundant, which is worth stating because it looks like they are. spctl asks Gatekeeper, and Gatekeeper will fetch the ticket from Apple over the network, so an artifact that was notarized but never successfully stapled still passes. Measured on this machine:
Ghostty is notarized, has no stapled ticket, and spctl accepts it anyway. Only
stapler validateproves the ticket is in the artifact, which is what a Mac with no network needs.build-release.shstapled and never validated, so a staple that silently produced no ticket shipped.The assessment type is a parameter:
execfor the app,openfor the DMG and the plugin bundles, since a user opens those rather than launching them.Also dropped:
--apple-idand--team-idon the plugin submission. The keychain profile theapple-signingaction stores already carries both plus the password, which is why the app and DMG paths have always worked with--keychain-profilealone.build-plugin.shadditionally required anAPPLE_IDenv var and failed without it, so the workflow passed a secret that changed nothing.Verified
I have no Developer ID certificate locally, so the submission itself cannot run here. Two things I could verify, and did:
The spctl and stapler invocations, against genuinely notarized apps (above), and against a non-notarized bundle to confirm the check fails — an ad-hoc signed bundle returns
rejectedand the grep does not match.Every branch of the function, with stubbed
xcrun/spctl:"submit ok but Invalid" is why the exit-status check alone is not enough: notarytool has shipped versions that exit 0 on a rejected submission, and only
build-release.shwas checking the status line. And the argument order inoutput="$(...)" || submit_status=$?is load-bearing; written the other way roundsubmit_status=$?reads the assignment's status, which is always 0, and the log fetch becomes unreachable. That was already fixed once inbuild-release.shand the comment moves with the code.The plugin workflow built each architecture from scratch
The script has always accepted
both. Each invocation regenerates both Xcode projects and runsrm -rf build/DerivedData, so the second call threw away everything the first had compiled. Measured here onSQLiteDriverPlugin, one of the smaller ones:The second architecture costs 24s or 3s depending only on this. Same two zips, same names, same SHA-256 sidecars:
bothwas already the script's default and takes the same code path.cleanupin five scriptsThe roadmap said ten; five is the real count. The other five already use the terse
trap 'rm -rf "$WORK"' EXITand need no function. The five had an identicalBUILD_DIR="$(mktemp -d)", a three-linecleanup()and the same trap, plus an identicalNCPU=$(sysctl -n hw.ncpu). All five already sourcelib/macos.sh, somake_build_dirandNCPUgo there.build-freetds.shkeeps its own trap: it removes a tarball cache as well, so it is not the same shape.Verified
build-libssh2.sh arm64run end to end after the change. The rebuilt library against the published one:The archive hash differs because a static archive embeds timestamps; the arch, symbol count and full exported symbol set match.
Libs/was then restored withdownload-libs.sh --forceand re-verified against the checksums committed in git.The trap fires on a clean exit and on a failure:
ThirdPartyLicenseInventoryTestsscansscripts/build-*.shandscripts/lib/*.shfor version pins, and I broke it once with a change of this shape. The pin set is byte-identical before and after: 10 pins, no diff.shellcheck -x --severity=warningandbash -nclean on every touched script;actionlintclean.