ci(desktop): assert the executable the app bundle declares - #5351
Merged
Merged
Conversation
The macos widget + bundle job's Verify step asserted Contents/MacOS/OpenCodex, but Tauri renames the main binary only when mainBinaryName is set. desktop/src-tauri/tauri.conf.json does not set it, so the bundled executable keeps the Cargo bin name opencodex-desktop and that assertion could never pass. The step had never run before: every earlier attempt died in the build ahead of it on the missing updater signing key, which #5338 has now separated out. With the build green the Verify step ran for the first time and failed on its first line with no output, because test is silent. Read CFBundleExecutable from the bundle's own Info.plist rather than restating the name, so the check follows the config instead of drifting from it, and reject an empty value so a missing key cannot pass by testing a directory. The other three assertions are unchanged.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
github-actions
Bot
added
the
chore
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe macOS widget verification now reads ChangesWidget CI validation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The executable verification fix is ready to merge. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Contributor
|
✅ Deterministic PR hygiene checks passed. |
Owner
Author
리뷰 · 우선순위 28 / 80이 PR은 macOS 데스크톱 CI의 Verify 단계가 틀리게 잡은 실행 파일 이름을 고칩니다. 예전에는 라인 - 라인 - 같은 Verify 스텝의 appex·ocx 라인 - 라인 - 업데이터 번들 경로는 #5338 이후 CI에서 안 돌아요. PR이 적어 둔 대로 릴리즈 쪽 가드가 있지만, 메인테이너의 판단이 필요한 지점 실행 파일 이름을 앞으로도 plist만 따라갈지, 아니면 너의 추천 이 head로 이 댓글은 grok-bot이 작성했습니다 |
lidge-jun
added a commit
that referenced
this pull request
Sep 20, 2026
The executable-name half of this landed separately as #5351, in a better form: it reads CFBundleExecutable from the bundle rather than restating the name, so it follows the config instead of drifting from it. What remains here is the assertion that has no equivalent. A widget extension with no widget in it is indistinguishable from a working one by every other check in this job: the appex builds, the signature verifies, pluginkit registers it, and the gallery is simply empty. That is what shipped, and it shipped silently. Reading the WidgetBundle symbol out of the binary is the only place in the build where its absence is visible.
lidge-jun
added a commit
that referenced
this pull request
Sep 20, 2026
…ime (#5339) * fix(desktop): give the widget an entry point and sign it at release time The WidgetKit extension installed, registered with pluginkit, and was never offered in the gallery. Signing looked like the cause and was not: the released build is signed and notarized and the widget is missing there too. app/Package.swift forced the executable's entry to _NSExtensionMain and main.swift was a comment, so nothing referenced OpenCodexWidgetBundle and no code handed it to the extension host. The shipped binary shows it: LC_MAIN pointing at _NSExtensionMain, SnapshotProvider present and the bundle absent. pluginkit registers from the Info.plist, which is complete, so registration succeeded; NSExtensionMain then looked for an NSExtensionPrincipalClass that a SwiftUI widget does not declare, because Xcode's @main on the WidgetBundle is what connects it instead. Nothing errored and the gallery had nothing to offer. main.swift now calls OpenCodexWidgetBundle.main(), which is what @main expands to, and the override is gone. Separately, release.yml ran build-widget.sh with no env block while MACOS_SIGN_IDENTITY was set one step later on the Tauri build, which never reads it, so the script took its ad-hoc branch on every release. The bundler does not re-sign anything under PlugIns - its nested-code walker knows .framework, .xpc and .app, not .appex - so an ad-hoc extension with no team identifier shipped inside a Developer ID host. The certificate is now imported before the widget build and the keychain deleted in an always() step, the widget build receives the identity, build-widget.sh adds --options runtime alongside --timestamp, and a following step asserts strict verification, the team identifier, the runtime flag and a timestamp instead of printing the signature. tests/clients/desktop-widget-entry.test.ts holds the entry point and was driven red by reinstating the linker override. The signing half needs maintainer credentials and a clean install to prove; that is recorded as outstanding rather than claimed. * feat(desktop): turn Start at Login on once, the first time an installation runs A menu bar app that is not running has no menu bar item, so leaving Start at Login off by default meant an installed app was simply absent after the next reboot, with nothing on screen to explain why. That is not a neutral default for an app whose main surface is the menu bar. first_run::apply_start_at_login_default runs once per installation, keyed on a marker in the app config directory. The marker is written before the login item is touched and is never removed, so a user who turns the setting back off keeps it off: the next launch sees the marker and does nothing. Writing afterwards instead would let a failed enable retry on every launch and eventually flip the setting back on under someone who had deliberately turned it off. Every failure is silent, because being unable to register a login item is not a reason to stop the app from starting. It runs before tray::install so the tray's Start at Login checkbox reads the state this leaves behind. The behaviour is not macOS-gated: the autostart plugin implements Linux autostart entries and the current-user Windows Run registration too. tests/clients/desktop-start-at-login-default.test.ts reads the ordering out of the source, because that ordering is the entire contract and is invisible from behaviour alone. * docs(devlog): record the verified widget entry and login-item evidence * fix(desktop): declare the widget's platform and display name Every widget macOS ships declares CFBundleSupportedPlatforms and CFBundleDisplayName. Ours declared neither, because Xcode writes both and a SwiftPM-assembled appex has no build system to write them. An extension bundle that does not say which platform it supports gives the system no reason to consider it on this one. Checked against the widgets on a macOS 27 install: Shortcuts, Tips and Reminders all carry CFBundleSupportedPlatforms = [MacOSX], and the two this bundle lacked are the only structural Info.plist differences between them and this one. * fix(desktop): give the widget both halves of an Xcode app-extension entry A widget extension needs @main on the WidgetBundle and the _NSExtensionMain linker entry, and either one alone produces a widget that is never offered in the gallery. Without @main nothing references the bundle, the linker drops it, and the extension still registers with pluginkit because the Info.plist alone is enough - so the gallery has no configuration to offer and nothing anywhere reports a problem. That is what shipped. Without the entry override the Swift main runs instead of the extension host's bootstrap and ExtensionFoundation traps in _EXRunningExtension._shared. Measured on a real install: EXC_BREAKPOINT on every launch, one crash report per attempt, and chronod logging "query failed - will try lazy reload later" while the gallery stayed empty. With both in place the crash reports stop and chronod processes the extension normally. The deployment target moves to macOS 14, which drops the now-redundant per-declaration availability guards and puts the binary's minos at 14.0, matching the working widgets on the machine this was measured on. Also recorded: the app-sandbox entitlement is not optional. Removing it does not fail at launch - pkd refuses to register the bundle at all, saying "plug-ins must be sandboxed", which is why the host writes its snapshot into the extension's own container. * ci(release): require every Mach-O in the bundle to carry the release identity Signing the extension by name is not enough. A bundler signs what it placed and nothing else, and the binaries that get missed in practice are the ones with no extension to filter on - so a check that names paths will keep passing while an unsigned executable rides along inside the bundle and notarization rejects the whole submission. This walks the built app, identifies executables by their Mach-O magic bytes rather than by path or suffix, and fails the job naming any file that does not carry the configured team identifier. Without a configured team it says so and skips, so a fork still builds and still cannot pretend to be signed. * docs(devlog): plan the stack landing and record the two dev repairs it carries * fix(release): repair the artifact upload and make the signing checks mean something Review caught four defects in the previous revision of this workflow change, one of them mine and serious. retention-days had been displaced out of the desktop upload action and into the cleanup step's shell body, where it is not a setting but a command that does not exist. Every macOS release job would have failed with "retention-days:: command not found", and the artifact would have taken the default retention on the way there. It is back in the action's with block. The keychain path is now recorded before the keychain is created, so a failure part way through still leaves the cleanup step something to delete, and the decoded certificate is removed by a trap rather than by a line a failure can skip past. The bundle-wide Mach-O sweep ran before Tauri produced a bundle, so it inspected nothing and passed. It now runs after the assets are collected and fails when it finds no app bundle or no Mach-O at all - a sweep that examined nothing is exactly the outcome it exists to prevent. All three signing checks were fail-open: with the secrets absent they printed a note and exited zero, while the unsigned artifact was still uploaded and attached. They now refuse a non-dry-run release outright and keep the permissive path only for a local or dry run. The step comment also claimed prior releases had shipped an ad-hoc extension inside a notarized host. No release has published a macOS application, so the comment says what is true instead: a defect that had not yet reached anyone. * docs(structure): record the first-run login item and the widget's two-part entry structure/ ownership obliges the doc for an area to move with the source, and this branch changed two things the desktop-shell note did not describe. first_run.rs turns Start at Login on once per installation, with the marker written before the login item so a user who turns it back off keeps it off. The widget needs @main on the bundle and the _NSExtensionMain linker entry together, and the sandbox entitlement is mandatory because pkd refuses to register an unsandboxed plug-in at all - which is the reason the shell writes its snapshot into the extension's container rather than anywhere more obvious. * fix(release): check the notarization credentials as a set and widen the Mach-O sweep Two holes left by the previous revision. Only the certificate and the team id were guarded, so a real release missing APPLE_ID or APPLE_PASSWORD still ran: the Tauri CLI skips notarization without failing when the notary credentials are absent, and the unnotarized artifact is uploaded and attached exactly as a good one would be. The five credentials are now checked together, and a non-dry-run release stops with the missing names rather than shipping something that looks finished. The bundle sweep recognised four of the eight Mach-O leading words, so a fat 64-bit or big-endian binary was skipped in silence while the other files kept the inspected-something counter healthy. All eight are listed now. The public install guidance is left alone on purpose, and the landing note says why: those pages describe an artifact that does not exist yet, and they should move with the first notarized release rather than ahead of it. * ci(desktop): assert that the widget bundle is linked into the extension The executable-name half of this landed separately as #5351, in a better form: it reads CFBundleExecutable from the bundle rather than restating the name, so it follows the config instead of drifting from it. What remains here is the assertion that has no equivalent. A widget extension with no widget in it is indistinguishable from a working one by every other check in this job: the appex builds, the signature verifies, pluginkit registers it, and the gallery is simply empty. That is what shipped, and it shipped silently. Reading the WidgetBundle symbol out of the binary is the only place in the build where its absence is visible. * fix(desktop): build the widget in extension-only mode Xcode sets APPLICATION_EXTENSION_API_ONLY on an app-extension target, and SwiftPM has no such target, so the flag has to be passed by hand. The two projects that have a SwiftPM-built widget extension working both do exactly this alongside the _NSExtensionMain linker entry, and the public report of the ExtensionFoundation crash this branch hit traces it to precisely the setup SwiftPM cannot express. Verified on a real install after the change: the extension registers, chronod captures its descriptors, and no crash report is produced. The devlog records what the same report settles - ad-hoc signing does not block the gallery, App Groups do not work ad-hoc so writing into the extension's own container is the documented fallback, and the host and extension CFBundleVersion must match, which they do. * fix(tests): bound the release injection guard at the job it is reading The guard splits the workflow on step names and scans from each "run: |" to the end of the block, which for the last step of a job runs on into the next job's header. Adding a cleanup step at the end of package-desktop made it read attach-release's job-level "if: inputs.dry-run != true" as shell interpolation inside the step above it - a condition, not a script, and not reachable by injection. Each block is now cut at the first line that dedents to job level, which is where the step's script actually ends. The guard still fails on a real interpolation: driven red by putting an inputs expression inside a run block, and green again once removed. * test(clients): hold the extension-only build flag the same way as the linker entry -application-extension is load-bearing and nothing asserted it: the existing nm check proves the WidgetBundle was linked, which stays true with the flag removed, and the runtime crash it prevents leaves the build, the signature and the pluginkit registration all looking fine. It sits beside the _NSExtensionMain assertion because the two are one contract - the projects that have a SwiftPM widget extension working supply both. structure/desktop-shell.md now records all three requirements together and says plainly that nothing observable distinguishes a broken one from a working widget. Driven red by deleting the flag. * docs(devlog): record that the widget now appears in the gallery * docs(devlog): correct the landing note for the three repairs that landed elsewhere
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
1 participant
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
The
macos widget + bundlejob still fails after #5338, now one step later. Its Verify step assertsContents/MacOS/OpenCodex, but Tauri renames the main binary only whenmainBinaryNameis set, anddesktop/src-tauri/tauri.conf.jsondoes not set it. The bundled executable therefore keeps the Cargo bin nameopencodex-desktop, and the assertion could never pass.This was invisible until now. Every earlier run died in the build ahead of it on the missing updater signing key, so the Verify step had never executed once. #5338 unblocked the build and the step ran for the first time, failing on its first line with no output at all —
testis silent, so the log shows onlyProcess completed with exit code 1.The fix reads
CFBundleExecutablefrom the bundle's ownInfo.plistinstead of restating the name in the workflow, so the check follows the config rather than drifting from it, and would keep working ifmainBinaryNamewere set later. An empty value is rejected so a missing key cannot pass bytest -xsucceeding on theMacOSdirectory. The appex, sidecar andcodesign -dvassertions are unchanged.Verification
Static review against the toolchain source, since local builds and suites are out of scope for this lane; exact-head hosted CI is the execution evidence.
7206312afa(run 35513146484): the build reportsFinished 1 bundle at: .../bundle/macos/OpenCodex.app, then the Verify step exits 1 with no output, which places the failure in one of the three silenttest -xlines rather thancodesign -dv.tauri-cli2.5.0 renames the main binary only underif let Some(main_binary_name) = &config.main_binary_name(src/interface/mod.rs), andrename_app(src/interface/rust/desktop.rs) is a no-op otherwise. The job log confirms it:Built application at: .../target/release/opencodex-desktop.tauri-bundler2.4.0copy_binaries_to_bundle(src/bundle/macos/app.rs) copies the main binary toContents/MacOS/<bin.name()>, andcreate_info_plistsetsCFBundleExecutablefrom the samemain_binary_name(), so the plist and the file on disk always agree.Settings::copy_binariesstrips the-<target>suffix, soocx-aarch64-apple-darwinlands asContents/MacOS/ocx;copy_custom_files_to_bundleresolvesbundle.macOS.filesrelative toContentsand errors loudly when the source is absent, so the appex is present andfs::copypreserves its executable bit..github/workflows/ci.ymlis not tracked by the file-size ratchet, so neither gate is affected.Checklist
Security review
This is a workflow change, so it is in scope for security review. It introduces no secret reference, reads no credential, and changes no
permissionsblock or job trigger./usr/libexec/PlistBuddyis an Apple system tool already present on the runner and reads a file the job just built. The verification build stays unsigned, and release signing remains entirely inrelease.yml, which this PR does not touch. No privilege escalation and no new secret exposure.Coverage note
Turning updater artifacts off in the verification build does leave the updater bundling path unexercised in CI, so a regression there would surface only during a release. Two release-time backstops already contain it:
collect-release-assets.tsthrows when the macOSapp.tar.gzis missing, andupdater-manifest.ts --require-allrefuses to publish a partially signedlatest.json. Both are covered bytests/ci-workflows/release-desktop-scripts.test.ts. Closing the remaining gap with a static contract test overtauri.conf.jsonis worth a follow-up but is not required to unreddev.Summary by CodeRabbit
Bug Fixes
Chores