Skip to content

Fix Sparkle signing and installer fallback#1962

Open
lawrencecchen wants to merge 5 commits intomainfrom
issue-1960-sparkle-update-hardening
Open

Fix Sparkle signing and installer fallback#1962
lawrencecchen wants to merge 5 commits intomainfrom
issue-1960-sparkle-update-hardening

Conversation

@lawrencecchen
Copy link
Copy Markdown
Contributor

@lawrencecchen lawrencecchen commented Mar 23, 2026

Summary

  • sign Sparkle and bundled helpers inside-out without --deep, using a shared codesign helper for nightly and release builds
  • show a manual latest-DMG fallback when Sparkle install errors block in-app updates
  • add regression coverage for both the signing workflow and the update error presentation

Testing

  • ./tests/test_ci_codesign_app_bundle.sh
  • xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -derivedDataPath /tmp/cmux-issue-1960-sparkle-update-hardening-tests -only-testing:cmuxTests/UpdateViewModelPresentationTests test
  • ./scripts/reload.sh --tag issue-1960-sparkle-update-hardening

Closes #1960
Closes #1961


Summary by cubic

Hardens macOS update flow by signing the app bundle inside‑out (no --deep) and adding a manual “Download Latest DMG” path when Sparkle install fails.

  • Bug Fixes

    • Hardened scripts/codesign_app_bundle.sh: resolves Sparkle.framework Versions/Current, signs XPCs/Autoupdate/Updater.app first, then other frameworks, then the app and bundled CLIs; avoids --deep and keeps app entitlements off Sparkle components.
    • Replaced ad‑hoc signing in release.yml, nightly.yml, and build-sign-upload.sh; CI runs tests/test_ci_codesign_app_bundle.sh (dry‑run) to verify signing order and flags.
  • New Features

    • On Sparkle installation errors, show “Download Latest DMG” in both the standard alert and the update popover; opens the correct stable or nightly DMG based on the resolved feed. Updated titles/messages and added tests for error mapping, URL selection (including nightly), code‑name details, and suppressing the link for network errors.

Written for commit f77e7d8. Summary will update on new commits.

Summary by CodeRabbit

  • New Features

    • Update UI now offers a "Download Latest DMG" flow when installation fails; popovers and updater prompts present the option plus retry/dismiss choices.
  • Localization

    • Added English and Japanese strings for the download action and installation-failed title/message.
  • CI

    • CI now runs an additional signing-validation step and delegates app-bundle signing to a centralized signing helper.
  • Tests

    • Added a CI regression test that validates the signing plan in dry-run mode.

@vercel
Copy link
Copy Markdown

vercel Bot commented Mar 23, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cmux Ready Ready Preview, Comment Mar 23, 2026 1:18am

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 23, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3c4c935e-ff2d-450c-ab62-c24117deec57

📥 Commits

Reviewing files that changed from the base of the PR and between 0c65f39 and f77e7d8.

📒 Files selected for processing (6)
  • Sources/Update/UpdateDriver.swift
  • Sources/Update/UpdatePopoverView.swift
  • Sources/Update/UpdateViewModel.swift
  • cmuxTests/ShortcutAndCommandPaletteTests.swift
  • scripts/codesign_app_bundle.sh
  • tests/test_ci_codesign_app_bundle.sh
✅ Files skipped from review due to trivial changes (2)
  • cmuxTests/ShortcutAndCommandPaletteTests.swift
  • Sources/Update/UpdateViewModel.swift
🚧 Files skipped from review as they are similar to previous changes (2)
  • Sources/Update/UpdatePopoverView.swift
  • Sources/Update/UpdateDriver.swift

📝 Walkthrough

Walkthrough

Centralizes macOS codesigning into a new signing script with CI regression tests for the signing plan, and adds a manual DMG download fallback in the Sparkle update UI and view model for a whitelist of installer error codes.

Changes

Cohort / File(s) Summary
CI Workflows
\.github/workflows/ci.yml, \.github/workflows/nightly.yml, \.github/workflows/release.yml
Replaced inline app-bundle codesign steps with calls to ./scripts/codesign_app_bundle.sh; added CI step to run signing regression test.
Build script
scripts/build-sign-upload.sh
Replaced inline bundle signing logic with invocation of ./scripts/codesign_app_bundle.sh.
Centralized Codesigning Script
scripts/codesign_app_bundle.sh
New executable: inside-out signing order, separate sign-without-entitlements and sign-with-entitlements helpers, argument validation, dry-run mode (CMUX_CODESIGN_DRY_RUN=1), and final verification.
Codesign CI Test
tests/test_ci_codesign_app_bundle.sh
New dry-run regression test that constructs a synthetic .app, captures dry-run signing plan, asserts presence/order of Sparkle/Sentry/CLI/helper signs, checks entitlement usage, and ensures verification includes --verify --deep --strict --verbose=2.
Update UI & Logic
Sources/Update/UpdateDriver.swift, Sources/Update/UpdatePopoverView.swift, Sources/Update/UpdateViewModel.swift
Offer manual DMG download for a whitelist of Sparkle installation error codes; UpdateDriver presents a manual-download alert when available; popover shows "Download Latest DMG" button; view model maps additional Sparkle error codes to install-failure title/message and exposes manualDownloadURL.
Localization & Tests
Resources/Localizable.xcstrings, cmuxTests/ShortcutAndCommandPaletteTests.swift
Added update.downloadLatestDmg, update.error.installationFailed.title/message (en/ja). Tests extended to cover Sparkle error titles/messages, manualDownloadURL resolution (stable vs nightly), and formatted Sparkle error-code details.

Sequence Diagram(s)

sequenceDiagram
    actor User
    participant UpdateDriver
    participant UpdateViewModel
    participant NSAlert
    participant NSWorkspace

    User->>UpdateDriver: Updater reports Sparkle installation error (e.g. SUSparkleErrorDomain 4005)
    UpdateDriver->>UpdateViewModel: manualDownloadURL(for: error)
    UpdateViewModel-->>UpdateDriver: URL or nil

    alt URL available
        UpdateDriver->>NSAlert: present manual-download alert (Download, Retry, OK)
        NSAlert-->>User: show choices
        User->>NSAlert: selects "Download Latest DMG"
        NSAlert->>NSWorkspace: open(manualDownloadURL)
        NSWorkspace-->>User: browser opens download URL
    else no URL
        UpdateDriver->>UpdateDriver: fallback to standard error presentation
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Poem

🐇 I hop through scripts with tiny paws,
signing Sparkle’s helpers without a pause.
If installer stumbles, fret no more—
a DMG awaits upon the shore.
Rabbit stamps "PASS" and hops out the door.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main changes: fixing Sparkle signing (inside-out approach without --deep) and adding an installer fallback UI.
Description check ✅ Passed The description covers Summary and Testing sections with specific test commands and issue references, though Demo Video section is missing.
Linked Issues check ✅ Passed The code changes fully address both issue requirements: #1960 implements inside-out signing without --deep via codesign_app_bundle.sh; #1961 adds manual DMG download UI fallback for Sparkle installer errors.
Out of Scope Changes check ✅ Passed All changes are scoped to the two linked issues: signing infrastructure, workflow updates, error handling, and test coverage; no unrelated modifications detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-1960-sparkle-update-hardening

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 11 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="Sources/Update/UpdateViewModel.swift">

<violation number="1" location="Sources/Update/UpdateViewModel.swift:7">
P2: Manual update fallback is hardcoded to the stable DMG, which sends NIGHTLY users to the wrong installer after Sparkle installation failures.</violation>
</file>

<file name="tests/test_ci_codesign_app_bundle.sh">

<violation number="1" location="tests/test_ci_codesign_app_bundle.sh:43">
P2: Avoid asserting literal workflow file text in this regression test; verify behavior through executable paths/artifacts instead.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread Sources/Update/UpdateViewModel.swift
Comment thread tests/test_ci_codesign_app_bundle.sh Outdated
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3814ad2919

ℹ️ 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".

import Sparkle

class UpdateViewModel: ObservableObject {
static let manualDownloadDMGURLString = "https://github.com/manaflow-ai/cmux/releases/latest/download/cmux-macos.dmg"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Choose manual DMG URL by update channel

The new fallback link is hardcoded to the stable asset (releases/latest/download/cmux-macos.dmg), but nightly builds use a different feed and asset (releases/download/nightly/appcast.xml and cmux-nightly-macos.dmg in .github/workflows/nightly.yml). That means NIGHTLY users who hit these Sparkle install errors and click “Download Latest DMG” are sent to the stable installer, which silently switches them off the nightly channel. Please derive the manual-download URL from the resolved update channel/feed instead of using a single stable URL.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@scripts/codesign_app_bundle.sh`:
- Around line 41-45: The sign_without_entitlements helper currently strips
entitlements for every target; detect the special-case XPC service by checking
for "Downloader.xpc" (e.g., basename of the path) and for that path perform
signing that preserves existing entitlements instead of the generic
no-entitlements flow: invoke emit_or_run (the same wrapper used here) with the
signing identity (SIGNING_IDENTITY) but include codesign's
entitlements-preserving option (--preserve-metadata=entitlements) so the XPC's
entitlements metadata is retained; for all other paths keep the existing
sign_without_entitlements behavior.

In `@tests/test_ci_codesign_app_bundle.sh`:
- Around line 88-100: Add a regression check for Downloader.xpc: after the block
that ensures Sparkle components are not signed with the app entitlements
(checking OUTPUT for "--entitlements"), add an assertion that Downloader.xpc
still has its own entitlements metadata preserved by verifying OUTPUT contains a
"--entitlements" entry for "Downloader.xpc" and that the entitlements path for
Downloader.xpc is not the app ENTITLEMENTS variable (OUTPUT and ENTITLEMENTS
should be used to locate the relevant lines). Ensure the check fails the script
if Downloader.xpc lacks any "--entitlements" entry or if its entitlements path
equals $ENTITLEMENTS.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: db2ebb6b-5be2-4eb2-9763-568cc12470ce

📥 Commits

Reviewing files that changed from the base of the PR and between c9616c5 and 3814ad2.

📒 Files selected for processing (11)
  • .github/workflows/ci.yml
  • .github/workflows/nightly.yml
  • .github/workflows/release.yml
  • Resources/Localizable.xcstrings
  • Sources/Update/UpdateDriver.swift
  • Sources/Update/UpdatePopoverView.swift
  • Sources/Update/UpdateViewModel.swift
  • cmuxTests/ShortcutAndCommandPaletteTests.swift
  • scripts/build-sign-upload.sh
  • scripts/codesign_app_bundle.sh
  • tests/test_ci_codesign_app_bundle.sh

Comment on lines +41 to +45
sign_without_entitlements() {
local path="$1"
[ -e "$path" ] || return 0
emit_or_run --force --options runtime --timestamp --sign "$SIGNING_IDENTITY" "$path"
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's verify the file exists and examine the relevant code sections
git ls-files | grep -E "codesign|sign"

Repository: manaflow-ai/cmux

Length of output: 4330


🏁 Script executed:

# Check the script at the specified lines
cat -n scripts/codesign_app_bundle.sh | sed -n '1,100p'

Repository: manaflow-ai/cmux

Length of output: 3413


🏁 Script executed:

# Search for all uses of the signing helpers to understand context
rg "sign_without_entitlements|sign_preserving_entitlements|Downloader\.xpc" scripts/

Repository: manaflow-ai/cmux

Length of output: 677


🌐 Web query:

Sparkle framework Downloader.xpc code signing requirements entitlements

💡 Result:

The Sparkle framework's Downloader.xpc is an optional XPC service used in sandboxed macOS apps without the com.apple.security.network.client entitlement for network downloads during updates. Code signing requirements: - Sign individually (do not use --deep): codesign -f -s "$IDENTITY" -o runtime --preserve-metadata=entitlements [path]/Sparkle.framework/Versions/B/XPCServices/Downloader.xpc (for Sparkle >=2.6; preserves existing entitlements). - For Sparkle <2.6 or manual signing: codesign -f -s "$IDENTITY" -o runtime --entitlements Entitlements/Downloader.entitlements [path]. Downloader.xpc entitlements: - Not sandboxed by default (as of Sparkle 2.6). - Optional sandboxing: requires building Sparkle from source, uncommenting DOWNLOADER_SANDBOXED_ENTITLEMENTS and modifying XPC_SERVICE_BUNDLE_ID_PREFIX in ConfigCommon.xcconfig. - Specific entitlements content (e.g., from Entitlements/Downloader.entitlements) enables network access (likely com.apple.security.network.client) but exact plist not found in searched sources; fetch from Sparkle repo for manual use. - Enable via Info.plist: SUEnableDownloaderService = true (only if app lacks network entitlement). App entitlements for XPC communication (separate from Downloader.xpc itself): com.apple.security.temporary-exception.mach-lookup.global-name $(PRODUCT_BUNDLE_IDENTIFIER)-spks $(PRODUCT_BUNDLE_IDENTIFIER)-spki Primary sources: Sparkle official docs (sandboxing), GitHub issues/PRs (#2062, steipete.me post).

Citations:


🌐 Web query:

codesign --preserve-metadata entitlements macOS command syntax

💡 Result:

The syntax for the macOS codesign command option --preserve-metadata related to entitlements is --preserve-metadata=entitlements. This option preserves the existing entitlement data from a prior signature when re-signing already-signed code. It requires --force (-f) to overwrite the existing signature. Full syntax example for re-signing while preserving entitlements: codesign --sign "Your Signing Identity" --force --preserve-metadata=entitlements [--options runtime] [--timestamp] /path/to/binary_or_bundle Other supported values for --preserve-metadata (comma-separated list): - identifier: Preserve signing identifier - requirements: Preserve internal requirements (including Designated Requirement) - flags: Preserve option flags - runtime: Preserve hardened runtime version - launch-constraints: Preserve launch constraints - library-constraints: Preserve library load constraints Without a value (deprecated), it preserves all known metadata. Use explicit lists for precision. To extract existing entitlements for verification: codesign -d --entitlements :- /path/to/binary (writes to stdout). This is used when re-signing to retain entitlements without needing to re-specify an entitlements file via --entitlements path, as seen in .NET apphost signing discussions and Apple Developer examples for DER entitlements.

Citations:


Special-case Downloader.xpc to preserve its existing entitlements during code signing.

Sparkle's official signing guide requires that Downloader.xpc preserve its entitlements metadata when re-signed for Sparkle 2.6+. Using the generic sign_without_entitlements path here strips that metadata, breaking the XPC service's functionality. (sparkle-project.org/documentation/sandboxing)

🛠️ Suggested change
 sign_without_entitlements() {
   local path="$1"
   [ -e "$path" ] || return 0
   emit_or_run --force --options runtime --timestamp --sign "$SIGNING_IDENTITY" "$path"
 }
 
+sign_preserving_entitlements() {
+  local path="$1"
+  [ -e "$path" ] || return 0
+  emit_or_run --force --options runtime --timestamp --sign "$SIGNING_IDENTITY" \
+    --preserve-metadata=entitlements "$path"
+}
+
 sign_with_entitlements() {
   local path="$1"
   [ -e "$path" ] || return 0
   emit_or_run --force --options runtime --timestamp --sign "$SIGNING_IDENTITY" --entitlements "$ENTITLEMENTS_PATH" "$path"
 }
@@
 if [ -n "$SPARKLE_VERSION_DIR" ]; then
   sign_without_entitlements "$SPARKLE_VERSION_DIR/XPCServices/Installer.xpc"
-  sign_without_entitlements "$SPARKLE_VERSION_DIR/XPCServices/Downloader.xpc"
+  sign_preserving_entitlements "$SPARKLE_VERSION_DIR/XPCServices/Downloader.xpc"
   sign_without_entitlements "$SPARKLE_VERSION_DIR/Autoupdate"
   sign_without_entitlements "$SPARKLE_VERSION_DIR/Updater.app"
 fi
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/codesign_app_bundle.sh` around lines 41 - 45, The
sign_without_entitlements helper currently strips entitlements for every target;
detect the special-case XPC service by checking for "Downloader.xpc" (e.g.,
basename of the path) and for that path perform signing that preserves existing
entitlements instead of the generic no-entitlements flow: invoke emit_or_run
(the same wrapper used here) with the signing identity (SIGNING_IDENTITY) but
include codesign's entitlements-preserving option
(--preserve-metadata=entitlements) so the XPC's entitlements metadata is
retained; for all other paths keep the existing sign_without_entitlements
behavior.

Comment thread tests/test_ci_codesign_app_bundle.sh Outdated
Comment on lines +88 to +100
for sparkle_component in Installer.xpc Downloader.xpc /Autoupdate Updater.app Sparkle.framework Sentry.framework; do
if printf '%s\n' "$OUTPUT" | grep -F "$sparkle_component" | grep -Fq -- '--entitlements'; then
echo "FAIL: $sparkle_component must not be signed with app entitlements"
exit 1
fi
done

for entitled_component in /Resources/bin/cmux /Resources/bin/ghostty /cmux.app; do
if ! printf '%s\n' "$OUTPUT" | grep -F "$entitled_component" | grep -Fv -- '--verify' | grep -Fq -- "--entitlements $ENTITLEMENTS"; then
echo "FAIL: $entitled_component must be signed with app entitlements"
exit 1
fi
done
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's examine the test file to understand the full context
find . -name "*test_ci_codesign_app_bundle*" -type f

Repository: manaflow-ai/cmux

Length of output: 98


🏁 Script executed:

# Also find the codesign_app_bundle.sh script
find . -name "codesign_app_bundle.sh" -type f

Repository: manaflow-ai/cmux

Length of output: 92


🏁 Script executed:

# Get the repository structure to understand the layout
git ls-files | head -20

Repository: manaflow-ai/cmux

Length of output: 675


🏁 Script executed:

# Read the test file to see full context around lines 88-100
wc -l ./tests/test_ci_codesign_app_bundle.sh

Repository: manaflow-ai/cmux

Length of output: 102


🏁 Script executed:

# Read the codesign script
wc -l ./scripts/codesign_app_bundle.sh

Repository: manaflow-ai/cmux

Length of output: 95


🏁 Script executed:

# Read the full test file
cat -n ./tests/test_ci_codesign_app_bundle.sh

Repository: manaflow-ai/cmux

Length of output: 4807


🏁 Script executed:

# Read the full codesign script
cat -n ./scripts/codesign_app_bundle.sh

Repository: manaflow-ai/cmux

Length of output: 3413


🌐 Web query:

Sparkle framework Downloader.xpc entitlements sandboxing --preserve-metadata

💡 Result:

In the Sparkle framework for macOS app updates, Downloader.xpc is an optional XPC service bundled in Sparkle.framework/Versions/B/XPCServices/ for sandboxed apps lacking the com.apple.security.network.client entitlement. It enables downloads without granting network access to the main app. By default (Sparkle 2.6+), Downloader.xpc is not sandboxed. To sandbox it (optional), build Sparkle from source by uncommenting DOWNLOADER_SANDBOXED_ENTITLEMENTS and setting XPC_SERVICE_BUNDLE_ID_PREFIX in ConfigCommon.xcconfig. For code signing in manual/custom workflows (e.g., non-Xcode), use --preserve-metadata=entitlements to retain any existing entitlements on Downloader.xpc: codesign -f -s "$CODE_SIGN_IDENTITY" -o runtime --preserve-metadata=entitlements "$APP_BUNDLE/Contents/Frameworks/Sparkle.framework/Versions/B/XPCServices/Downloader.xpc" This preserves sandbox entitlements if present (e.g., from source build). For Sparkle <2.6 or explicit application, use --entitlements Entitlements/Downloader.entitlements (Entitlements/ dir in Sparkle source for sandboxed Downloader). Xcode archive/export preserves entitlements automatically. Avoid --deep signing; sign components individually as entitlements differ (Downloader may be sandboxed, Installer.xpc is not). Enable via SUEnableDownloaderService=YES in app Info.plist if needed. Main app needs com.apple.security.temporary-exception.mach-lookup.global-name for XPC communication.

Citations:


Add a regression check for Downloader.xpc's preserved entitlements.

This test only proves that Downloader.xpc does not inherit the app entitlements. It does not lock in Sparkle's documented special case for preserving Downloader.xpc's own entitlements metadata, so CI would still stay green if the helper regressed there. (sparkle-project.org)

🧪 Suggested assertion
-for sparkle_component in Installer.xpc Downloader.xpc /Autoupdate Updater.app Sparkle.framework Sentry.framework; do
+for sparkle_component in Installer.xpc /Autoupdate Updater.app Sparkle.framework Sentry.framework; do
   if printf '%s\n' "$OUTPUT" | grep -F "$sparkle_component" | grep -Fq -- '--entitlements'; then
     echo "FAIL: $sparkle_component must not be signed with app entitlements"
     exit 1
   fi
 done
+
+if ! printf '%s\n' "$OUTPUT" | grep -F 'Downloader.xpc' | grep -Fq -- '--preserve-metadata=entitlements'; then
+  echo "FAIL: Downloader.xpc must preserve its own entitlements when re-signed"
+  exit 1
+fi
+
+if printf '%s\n' "$OUTPUT" | grep -F 'Downloader.xpc' | grep -Fq -- "--entitlements $ENTITLEMENTS"; then
+  echo "FAIL: Downloader.xpc must not inherit app entitlements"
+  exit 1
+fi
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/test_ci_codesign_app_bundle.sh` around lines 88 - 100, Add a regression
check for Downloader.xpc: after the block that ensures Sparkle components are
not signed with the app entitlements (checking OUTPUT for "--entitlements"), add
an assertion that Downloader.xpc still has its own entitlements metadata
preserved by verifying OUTPUT contains a "--entitlements" entry for
"Downloader.xpc" and that the entitlements path for Downloader.xpc is not the
app ENTITLEMENTS variable (OUTPUT and ENTITLEMENTS should be used to locate the
relevant lines). Ensure the check fails the script if Downloader.xpc lacks any
"--entitlements" entry or if its entitlements path equals $ENTITLEMENTS.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
Sources/Update/UpdateViewModel.swift (1)

348-354: The edge case is handled gracefully but relies entirely on CI injection of SUFeedURL for nightly builds.

When SUFeedURL is missing, UpdateFeedResolver.resolvedFeedURLString intentionally falls back to the stable appcast feed and sets isNightly = false. This is tested behavior. However, nightly builds depend entirely on the build script (scripts/build-sign-upload.sh:90) injecting the correct nightly appcast URL with /nightly/ in the path for the isNightly detection to work. If that injection fails, a nightly build would request the stable DMG. While unlikely in CI-controlled production builds, it might be worth adding a secondary heuristic (e.g., checking CFBundleVersion for nightly markers) as a belt-and-suspenders approach.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Sources/Update/UpdateViewModel.swift` around lines 348 - 354,
manualDownloadURL(for:) currently relies solely on SUFeedURL +
UpdateFeedResolver.resolvedFeedURLString to decide isNightly; add a fallback
heuristic that checks Bundle.main.object(forInfoDictionaryKey:
"CFBundleVersion") (or another infoplist key) for a nightly marker (e.g.,
contains "nightly" or a known nightly suffix/prefix) when infoFeedURL is nil or
UpdateFeedResolver returns .isNightly == false; update the logic in
UpdateViewModel.manualDownloadURL(for:) to treat the build as nightly if either
UpdateFeedResolver.resolvedFeedURLString(infoFeedURL: infoFeedURL).isNightly OR
the CFBundleVersion heuristic matches, so nightly DMG URL is returned even if
SUFeedURL injection failed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@Sources/Update/UpdateViewModel.swift`:
- Around line 348-354: manualDownloadURL(for:) currently relies solely on
SUFeedURL + UpdateFeedResolver.resolvedFeedURLString to decide isNightly; add a
fallback heuristic that checks Bundle.main.object(forInfoDictionaryKey:
"CFBundleVersion") (or another infoplist key) for a nightly marker (e.g.,
contains "nightly" or a known nightly suffix/prefix) when infoFeedURL is nil or
UpdateFeedResolver returns .isNightly == false; update the logic in
UpdateViewModel.manualDownloadURL(for:) to treat the build as nightly if either
UpdateFeedResolver.resolvedFeedURLString(infoFeedURL: infoFeedURL).isNightly OR
the CFBundleVersion heuristic matches, so nightly DMG URL is returned even if
SUFeedURL injection failed.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a51a680f-3795-40f8-be12-5b097afca89f

📥 Commits

Reviewing files that changed from the base of the PR and between 3814ad2 and 64474cc.

📒 Files selected for processing (3)
  • Sources/Update/UpdateViewModel.swift
  • cmuxTests/ShortcutAndCommandPaletteTests.swift
  • tests/test_ci_codesign_app_bundle.sh
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/test_ci_codesign_app_bundle.sh

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 64474cc51e

ℹ️ 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".

sign_without_entitlements() {
local path="$1"
[ -e "$path" ] || return 0
emit_or_run --force --options runtime --timestamp --sign "$SIGNING_IDENTITY" "$path"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve Sparkle entitlements when re-signing helpers

This helper re-signs Sparkle components via --force --sign without --entitlements or --preserve-metadata=entitlements, which means the existing signature metadata is not retained during release/nightly signing. As a result, Sparkle’s nested helpers (Installer/Downloader/Updater) can lose their embedded entitlements, contradicting the script’s stated goal to keep Sparkle helper entitlements and potentially reintroducing update-install failures in signed production builds.

Useful? React with 👍 / 👎.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 23, 2026

Greptile Summary

This PR addresses two related issues with Sparkle auto-updates: it consolidates all codesigning into a shared helper script that signs the bundle inside-out (Sparkle XPC helpers first, then the framework, then other dependencies, then the app) without --deep, and it adds a "Download Latest DMG" fallback UI path that appears when Sparkle installation errors (codes 4000–4012) block in-app updates. Both flows are backed by regression tests.

Key changes:

  • scripts/codesign_app_bundle.sh: New shared signing helper replaces duplicated inline codesign blocks in nightly.yml, release.yml, and build-sign-upload.sh. Signs Sparkle internals without app entitlements, preventing entitlement pollution that was the root cause of issue Fix codesign --deep applying wrong entitlements to Sparkle XPC services #1960.
  • UpdateViewModel.swift: Remaps Sparkle error codes 4000–4012 from the old "permission error" bucket to a new "installation failed" bucket, and adds manualDownloadURL(for:) / shouldOfferManualDownload(for:) to gate the fallback on only installation-class errors.
  • UpdateDriver.swift: Presents a native NSAlert with "Download Latest DMG / Retry / OK" buttons when an installation error occurs in the standard (dialog) presentation path.
  • UpdatePopoverView.swift: Adds the same "Download Latest DMG" button to the custom update error popover.
  • Tests: tests/test_ci_codesign_app_bundle.sh validates signing order, entitlement assignments, and absence of --deep via dry-run; three new XCTest cases cover error copy, URL presence, and network-error exclusion.

Confidence Score: 5/5

  • Safe to merge — the signing refactor is well-tested, the fallback UI is guarded to installation-error codes only, and the only open items are non-blocking style improvements.
  • Both the signing fix and the update fallback UI are covered by regression tests. The consolidated codesign helper is a strict improvement over the duplicated inline blocks — it removes --deep (the root cause of the signing bug) and is validated by a dry-run test that checks ordering, entitlement correctness, and workflow adoption. The open comments are P2 style issues (missing symbolic names for 4000-range Sparkle codes, duplicated error code set, symlink resolution clarity) that do not affect correctness or production reliability.
  • No files require special attention.

Important Files Changed

Filename Overview
scripts/codesign_app_bundle.sh New shared codesign helper that replaces inline codesign calls across all three build workflows. Signs Sparkle nested components (XPC services, Autoupdate, Updater.app) first without app entitlements, signs the Sparkle framework, signs remaining frameworks/bundles/dylibs, then signs the CLI helpers and the app bundle with entitlements — the correct inside-out order. The --deep flag is intentionally absent. Dry-run mode enables CI regression testing without a real signing identity.
Sources/Update/UpdateViewModel.swift Extends Sparkle installation-error handling (codes 4000–4005, 4010, 4012) with new user-facing title/message strings and a manualDownloadURL(for:) helper that returns the latest-release DMG URL for those codes. sparkleErrorCodeName does not yet include the new 4xxx codes, producing numeric-only debug output for those errors.
Sources/Update/UpdateDriver.swift Adds an NSAlert-based manual-download fallback in showUpdaterError for standard-presentation flows when manualDownloadURL returns a URL. The three-button alert (Download DMG / Retry / OK) correctly calls acknowledgement() in all paths and dispatches a fresh checkForUpdates only when the user chooses Retry.
Sources/Update/UpdatePopoverView.swift Adds a conditionally-rendered "Download Latest DMG" button to UpdateErrorView when manualDownloadURL is non-nil. Button correctly dismisses the popover and calls error.dismiss() before opening the URL.
tests/test_ci_codesign_app_bundle.sh CI regression test that builds a fake app bundle, runs the codesign helper in dry-run mode, and validates signing order, entitlement assignment, and absence of --deep. Also verifies that all three call sites in workflows/scripts delegate to the shared helper. Missing one ordering assertion: cli_line < helper_line.
cmuxTests/ShortcutAndCommandPaletteTests.swift Adds three unit tests covering the new installation-error copy, manual download URL presence for Sparkle errors, and absence of the URL for network errors. Good regression coverage.

Sequence Diagram

sequenceDiagram
    participant Sparkle
    participant UpdateDriver
    participant UpdateViewModel
    participant NSAlert
    participant UpdateErrorView

    Sparkle->>UpdateDriver: showUpdaterError(error)
    UpdateDriver->>UpdateViewModel: manualDownloadURL(for: error)

    alt Standard presentation & installation error (4000-4012)
        UpdateViewModel-->>UpdateDriver: URL("…/cmux-macos.dmg")
        UpdateDriver->>NSAlert: presentManualDownloadAlert(error, url)
        NSAlert-->>UpdateDriver: .alertFirstButtonReturn (Download DMG)
        UpdateDriver->>NSWorkspace: open(manualDownloadURL)
        UpdateDriver->>Sparkle: acknowledgement()
    else Standard presentation, other error
        UpdateViewModel-->>UpdateDriver: nil
        UpdateDriver->>Sparkle: standard.showUpdaterError(error)
    else Custom presentation
        UpdateViewModel-->>UpdateDriver: (not called)
        UpdateDriver->>UpdateViewModel: setState(.error(...))
        UpdateViewModel-->>UpdateErrorView: render error state
        UpdateErrorView->>UpdateViewModel: manualDownloadURL(for: error)
        alt Installation error
            UpdateViewModel-->>UpdateErrorView: URL("…/cmux-macos.dmg")
            UpdateErrorView->>NSWorkspace: open(manualDownloadURL)
        end
    end
Loading

Comments Outside Diff (3)

  1. Sources/Update/UpdateViewModel.swift, line 364-381 (link)

    P2 Missing names for new 4xxx error codes in sparkleErrorCodeName

    sparkleErrorCodeName doesn't include any of the Sparkle installation error codes that were just added to the rest of the error-handling switch statements (4000, 4001, 4002, 4003, 4004, 4010, 4012). When errorDetails formats technical output for these errors, it will fall through to the else branch and emit only the raw numeric code (e.g. Code: 4005) instead of a human-readable name. This makes the debug log less useful when users paste the "Copy Details" output in a support report.

  2. Sources/Update/UpdateViewModel.swift, line 364-381 (link)

    P2 Installation error codes duplicated in three places

    The set 4000, 4001, 4002, 4003, 4004, 4005, 4010, 4012 is now identical across userFacingErrorTitle, userFacingErrorMessage, and shouldOfferManualDownload. If a new installation error code needs to be added (or an existing one removed), it must be updated in all three switch statements independently. Consider extracting a private constant to keep them in sync:

    private static let installationErrorCodes: Set<Int> = [4000, 4001, 4002, 4003, 4004, 4005, 4010, 4012]

    Then each switch can call installationErrorCodes.contains(nsError.code) instead of repeating the literal list.

  3. Sources/Update/UpdateViewModel.swift, line 364-381 (link)

    P2 sparkleErrorCodeName doesn't cover installation error codes

    The sparkleErrorCodeName function (used by errorDetails to annotate the "Code:" line in the technical details panel) has no entries for codes 4000–4012. As a result, users who copy technical details from an installation-failure error will see Code: 4005 instead of something like Code: SUInstallationCachePathError (4005). Since the 4000-range codes are now the primary user-visible failure path (and the ones that prompt the manual DMG download), it would be helpful to add at least the codes you're already handling:

    case 4000: return "SUInstallationError"
    case 4001: return "SUInstallationRootInteractiveError"
    case 4002: return "SUInstallationAuthorizeLaunchError"
    case 4003: return "SUInstallationTargetBundleVersionCheckError"
    case 4004: return "SUInstallationCanceledError"
    case 4005: return "SUInstallationCachePathError"
    case 4010: return "SUInstallationXPCServiceError"
    case 4012: return "SUInstallationPermissionError"

    (Verify names against the Sparkle header you're targeting.)

Reviews (1): Last reviewed commit: "Address Sparkle review follow-ups" | Re-trigger Greptile

Comment on lines +83 to +86
if ! printf '%s\n' "$OUTPUT" | grep -F "$entitled_component" | grep -Fv -- '--verify' | grep -Fq -- "--entitlements $ENTITLEMENTS"; then
echo "FAIL: $entitled_component must be signed with app entitlements"
exit 1
fi
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Ordering assertion missing cli_line < helper_line check

The compound ordering guard checks sentry_line < cli_line and helper_line < app_line, but skips checking that cli_line < helper_line. If the script were ever reordered so that bin/ghostty was signed before bin/cmux, this test would not catch the regression.

Suggested change
if ! printf '%s\n' "$OUTPUT" | grep -F "$entitled_component" | grep -Fv -- '--verify' | grep -Fq -- "--entitlements $ENTITLEMENTS"; then
echo "FAIL: $entitled_component must be signed with app entitlements"
exit 1
fi
if [ "$sparkle_line" -ge "$sentry_line" ] || [ "$sentry_line" -ge "$cli_line" ] || [ "$cli_line" -ge "$helper_line" ] || [ "$helper_line" -ge "$app_line" ] || [ "$app_line" -ge "$verify_line" ]; then

Comment on lines +53 to +63
resolve_sparkle_version_dir() {
local sparkle_framework="$1"
local current_dir="$sparkle_framework/Versions/Current"

if [ -d "$current_dir" ]; then
printf '%s\n' "$current_dir"
return 0
fi

find "$sparkle_framework/Versions" -mindepth 1 -maxdepth 1 -type d ! -name Current -print | LC_ALL=C sort | head -n 1
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 resolve_sparkle_version_dir returns the Current symlink path, not the canonical path

When Versions/Current exists as a symlink (the normal case), the function returns $sparkle_framework/Versions/Current — the symlink itself — rather than resolving it to the concrete directory (e.g. Versions/B). While codesign follows symlinks on macOS and this won't cause signing failures in practice, it means the paths recorded in any log or dry-run output contain …/Versions/Current/… rather than …/Versions/B/…, which can be confusing when debugging. Consider resolving the symlink explicitly:

if [ -L "$current_dir" ]; then
  printf '%s\n' "$(readlink -f "$current_dir")"
  return 0
elif [ -d "$current_dir" ]; then
  printf '%s\n' "$current_dir"
  return 0
fi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Show manual download link when Sparkle installer fails Fix codesign --deep applying wrong entitlements to Sparkle XPC services

1 participant