Repository navigation
fix(ci): prune Qt's mangled nested .app bundles before signing - #25
Merged
Merged
Conversation
macOS release builds still fail at the signing step, now further in — past the dangling symlinks fixed in #24, after ~60 binaries sign successfully: .../PySide6/Assistant__dot__app/Contents/MacOS/Assistant: replacing existing signature .../Assistant__dot__app/Contents/MacOS/Assistant: the main executable or Info.plist must be a regular file (no symlinks, etc.) PyInstaller splits the bundle across Contents/Frameworks (binaries) and Contents/Resources (data), cross-linking each side, and relocates Qt's nested tool bundles into Frameworks under a mangled "<name>__dot__app" name. The directory is real but its Contents/Info.plist is a symlink into Resources. codesign, handed a Mach-O at <X>/Contents/MacOS/<n>, signs <X> as a bundle — and refuses outright when that bundle's Info.plist is a symlink. Assistant is Qt's help viewer. This app links QtCore/QtGui/QtWidgets only, and QtHelp — the module that would drive Assistant — is already excluded from the build, so nothing can launch it. Pruning the mangled nested bundles fixes signing and drops dead weight from every macOS artifact. QtWebEngineProcess is spared by name: it is the one nested helper Qt needs at runtime, if WebEngine ever returns to the build. macos-sign.sh now also names the file and explains this specific failure instead of dying on a bare codesign error, so the next occurrence is self-diagnosing. Verified against a bundle reproducing the real Frameworks/Resources split, with a codesign stub that refuses a symlinked Info.plist the way Apple's does: the pre-fix script fails with CI's exact message, and with the prune the Assistant bundle is removed from both roots, the WebEngine helper survives and signs, and the bundle signs clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JDuaD9Tgz6PT1yUypGzAmU
|
You have reached your Codex usage limits for security reviews. Please try again later. |
|
Warning Review limit reached
Next review available in: 29 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cecc877a8a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review finding, and a fair one: sparing QtWebEngineProcess__dot__app protected nothing. That helper has the same PyInstaller-split layout and symlinked Contents/Info.plist as Assistant, so if it were ever present the signing pass would walk into it and fail with the identical codesign error this change exists to fix — the carve-out only guaranteed the failure it claimed to prevent. My own test fixture hid this by giving the helper a real Info.plist, which is not how PyInstaller lays it out. Not pruning it either, though: Qt needs it at runtime, so pruning trades a loud build failure for a silently broken WebEngine at the user's end. It cannot appear today (every WebEngine module is excluded), so the build now stops with an explanation of what re-enabling WebEngine actually requires — rebuilding the helper into a signable layout — which is the one path that neither ships something broken nor fails later inside codesign. Tested both branches with a fixture that reproduces the real layout: Assistant-only prunes and signs clean, and a re-enabled WebEngine helper fails in the prune step with the explanation rather than in codesign. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JDuaD9Tgz6PT1yUypGzAmU
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cde5dcb5b0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Partly adopting a review finding. The finding's premise — that `-maxdepth` is GNU-only and unsupported by Apple's find(1) — is not right: it is a BSD primary, documented in macOS's own find(1) man page, inherited from FreeBSD, and this workflow already relies on it a few lines up. No change needed there. The second half of the finding is correct and worth fixing, though: a find(1) failure inside a process substitution does not propagate under set -e. Whatever the cause, a scan that failed would sweep nothing, the step would report success, and the build would die later inside codesign with nothing pointing back here. Capturing via command substitution puts find's exit status back in reach, so a failed scan stops the build where the problem is. Branches covered by the fixture: Assistant-only prunes and signs clean; a WebEngine helper stops the build with the explanation and is left in place; a bundle with no nested apps is a clean no-op; and a find that fails now exits 1 with a diagnostic where it previously reported success with the bundle untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JDuaD9Tgz6PT1yUypGzAmU
pacnpal
added a commit
that referenced
this pull request
Aug 28, 2026
* release: v1.3.2 — signed and notarized macOS builds The Developer ID signing + notarization pipeline landed in #23 (with follow-up fixes in #24 and #25) after v1.3.1 was cut, so every published macOS zip is still the old ad-hoc-signed build. Nothing ships it until a `v*` tag is pushed: release.yml only publishes assets on a tag push, and the 2026-08-20 dispatch smoke-test on this commit confirmed sign + notarize + staple succeed on macos-arm64, macos-x86_64, and macos-universal2. - Bump the package version to 1.3.2. It had been left at 1.2.6 through the v1.3.0 and v1.3.1 tags, and it is not cosmetic: `__version__` is read from installed package metadata and stamped into every disc's boot menu (`MENU TITLE FloppyBootCD vX.Y.Z`), so discs built from v1.3.1 identify themselves as v1.2.6. Also surfaced by `--version` and the About dialog. - Rewrite the macOS install docs now that a signed build exists to point at. README, the Pages site, and `.postbeep/docs.md` all told every macOS user to strip the quarantine flag; that step is unnecessary from v1.3.2 on, and `.postbeep/docs.md` (rendered on POSTBEEP.NET) still described the app as flatly "unsigned". Each now leads with the plain unzip-and-open path plus the `spctl` check, and keeps the `xattr` workaround scoped to v1.3.1 and older. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016NNtwTQvf8G9hEuEsHwQBH * fix(ci): fail a tag build closed when signing credentials are missing Review caught a real gap: the docs added in the previous commit promise that every macOS artifact from v1.3.2 on is signed and notarized, but release.yml could publish an ad-hoc-signed one. macos-import-cert.sh and macos-notarize.sh both exit 0 when their secrets are absent, and the "Locate signing tooling" step only warns when the tooling is missing — so a release cut with a lapsed or misconfigured secret would ship a bundle Gatekeeper blocks, under a promise that it wouldn't. That graceful degradation is right for fork PRs and dispatch smoke-tests and is kept. It is wrong for a release, which is the one trigger that publishes. Introduce a single MACOS_SIGNING_REQUIRED contract, set at the workflow level for pushed v* tags only, and honour it everywhere the fallback lives: - macos-import-cert.sh, macos-sign.sh, macos-notarize.sh: each turns its clean no-op into a hard failure with a message naming the missing secret. - "Locate signing tooling" (both the matrix job and universal2): missing tooling is fatal on a tag build, since every signing step below is gated on its output and would otherwise silently skip. A release now either carries a real Developer ID signature and a stapled ticket, or it doesn't happen — which is what makes the docs' claim unconditional rather than best-effort. Also from review: - docs/index.html: the spctl example checked the relative path, but the block moves the app to /Applications first, so following it in order made the check fail. Point it at the installed path. - README: the legacy xattr command sat in the same copy-pasteable block as the current-release steps, so a v1.3.2 user working top to bottom still stripped quarantine. Move it into a collapsed "Downloading v1.3.1 or older?" section. - .github/signing/README.md: document the new contract. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016NNtwTQvf8G9hEuEsHwQBH --------- Co-authored-by: Claude <thewesker@gmail.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.
Follow-up to #24. macOS release builds still fail at the signing step — but further in. The dangling symlinks are gone and ~60 binaries sign successfully before this:
Cause
PyInstaller splits the
.appacrossContents/Frameworks(binaries) andContents/Resources(data), cross-linking each side — and relocates Qt's nested tool bundles intoFrameworksunder a mangled<name>__dot__appname. The directory is real, but itsContents/Info.plistis a symlink intoResources.Handed a Mach-O at
<X>/Contents/MacOS/<n>,codesigntreats<X>as a bundle and signs it as one — then refuses when that bundle'sInfo.plistis a symlink. There is no flag to sign the file without the enclosing bundle, so the layout is simply unsignable as shipped.Fix
Prune the mangled nested bundles on macOS. Assistant is Qt's help viewer; this app links QtCore/QtGui/QtWidgets only, and
QtHelp— the module that would drive Assistant — is already--exclude-module'd, so nothing in the bundle can launch it. Removing them fixes signing and drops dead weight from every macOS artifact.QtWebEngineProcessis spared by name: it's the one nested helper Qt genuinely needs at runtime, should WebEngine ever return to the build.macos-sign.shalso now names the failing file and explains this specific case instead of dying on a barecodesignerror, so a future occurrence diagnoses itself.Testing
Against a bundle reproducing the real Frameworks/Resources split, with a
codesignstub that refuses a symlinkedInfo.plistthe way Apple's does:Assistant__dot__appremoved from both rootsQtWebEngineProcess__dot__appkept, signedStill unverified without a Mac: that the resulting bundle notarizes and launches.
Generated by Claude Code