Skip to content

Notarize serve-sim camera dylibs in macOS releases#7077

Merged
brennanb2025 merged 3 commits into
mainfrom
brennanb2025/investigate-6877-macos-notarization
Jul 2, 2026
Merged

Notarize serve-sim camera dylibs in macOS releases#7077
brennanb2025 merged 3 commits into
mainfrom
brennanb2025/investigate-6877-macos-notarization

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #6877.

This adds a macOS release-only afterSign step that submits the packaged standalone serve-sim libSimCameraInjector.dylib copies to Apple notarization via zipped xcrun notarytool submit --wait archives. The outer app notarization path is unchanged; this closes the gap where nested camera injector dylibs could be signed but not directly accepted by Gatekeeper assessment.

Important boundary: local validation cannot prove the final Apple Gatekeeper result without release credentials and a produced signed macOS artifact. Before release, run the macOS release pipeline and assess the shipped app plus nested dylib copies with spctl/codesign.

Design Approach

The smallest scoped change is in config/electron-builder.config.cjs: keep existing macOS app signing/notarization behavior, then add a release-gated hook for the nested standalone dylibs that stapler cannot ticket directly. The hook requires the two packaged resource copies currently declared by the build and notarizes the app.asar.unpacked copy only when present.

Design Review Outcome

Design review completed clean after tightening the test plan around behavior rather than source-string checks, and after explicitly treating release artifact spctl validation as a credentialed release gate.

Implementation

  • Added afterSign notarization for packaged serve-sim camera injector dylibs when ORCA_MAC_RELEASE=1 and electronPlatformName === 'darwin'.
  • Supports Apple ID/app password/team, App Store Connect API key, and keychain profile credential families.
  • Redacts credential values from wrapped subprocess failures.
  • Bounds ditto and notarytool --wait subprocesses so release jobs fail instead of hanging indefinitely.
  • Leaves non-release and non-macOS packaging inert.
  • Added focused tests for release gating, path coverage, credential mapping, missing files, bad notary responses, subprocess failure redaction, subprocess timeouts, and temp cleanup.

No UI was touched, so docs/STYLEGUIDE.md is not applicable.

Completeness

Completeness verification passed against the requested macOS release-packaging scope. The headline behavior is implemented locally, with the live signed-artifact Gatekeeper assessment remaining as the release-validation gap.

Consistency

Source-of-truth behavior is the macOS release packaging/notarization path in electron-builder. Neighboring surfaces are Windows/Linux packaging and local development packaging; those remain unchanged because the new hook exits outside macOS release builds.

Risks And Non-Goals

  • This does not attempt to staple bare dylibs; Apple stapler does not support that artifact shape.
  • This does not change serve-sim packaging layout beyond requiring the declared release dylib copies to be present.
  • This does not prove Apple acceptance for a future release artifact until CI or a credentialed macOS machine runs the live release pipeline and spctl checks.

Performance

Performance audit found no runtime hot path. The new work is bounded to macOS release packaging and runs two required, plus one optional, ditto/notarytool submissions.

Code Review

The code-review loop completed with a fresh final round of two reviewers. Both returned clean. An earlier actionable finding was fixed by making the app.asar.unpacked dylib optional instead of hard-required.

Merge-Confidence Validation

Result: land after release-artifact validation, or land now only if the live Apple artifact gap is explicitly accepted.

Passed locally:

  • pnpm exec vitest run --config config/vitest.config.ts config/scripts/electron-builder-config.test.mjs — 26 passed
  • pnpm exec oxfmt --check config/electron-builder.config.cjs config/scripts/electron-builder-config.test.mjs — passed
  • pnpm exec oxlint config/electron-builder.config.cjs config/scripts/electron-builder-config.test.mjs — passed
  • git diff --check — passed
  • pnpm run typecheck — passed
  • pnpm run lint — passed with existing unrelated warnings
  • pnpm build:unpack — passed locally; runner output was truncated because the build is noisy, but the command exited 0 and passed electron-builder packaging/config validation
  • Config import probe confirmed normal electron-builder imports do not expose the Vitest-only __test seam

Not applicable:

  • UI/Electron/demo-project screenshots, mobile, SSH, and agent/provider validation. This PR only touches macOS release packaging.

Release gate still needed:

  • Run a credentialed macOS release build.
  • Validate the shipped artifact and nested dylib copies with spctl -a -vvv and codesign --verify.

Post-PR Stabilization

  • CodeRabbit latest review: no actionable comments. Earlier timeout finding was addressed in commit 65a81fd.
  • PR checks: passing (verify, Ubuntu native smoke, Windows native smoke); other platform jobs are skipped by workflow rules.
  • Pushed-state verification: local branch matched upstream after the last push.

Made with Orca 🐋

Co-authored-by: Orca <help@stably.ai>
@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

This change adds macOS-specific notarization of serve-sim camera dylibs to the Electron Builder afterSign flow for production Mac release builds. It resolves packaged Contents/Resources paths, validates required dylibs, optionally includes unpacked dylibs, notarizes each file with notarytool, redacts credential values on subprocess errors, and exports internal helpers for tests. The test suite adds host mocking, environment overrides, and coverage for gating, credential mapping, missing files, and cleanup behavior.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main macOS release notarization change.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed Mostly complete and detailed, but it lacks explicit Screenshots, Security Audit, and a dedicated AI Review Report section.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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.

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 1

🧹 Nitpick comments (2)
config/scripts/electron-builder-config.test.mjs (2)

324-358: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider splitting into separate it blocks for clearer failure isolation.

This single test iterates four distinct scenarios (missing Apple ID password, missing API key trio, invalid notary status, invalid JSON output) via loops. If one sub-case fails, the reported failure won't clearly indicate which credential/output scenario broke.


360-387: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Redaction test only checks one secret value.

The assertion expect(error.message).not.toContain('app-password') verifies only the Apple app-specific password is redacted. Given appleIdMacReleaseEnv also includes APPLE_ID (an email, potentially PII) and APPLE_TEAM_ID, broadening the assertion to cover these values would give stronger confidence that redactCredentialValues scrubs all credential-derived args, not just the password.

✏️ Proposed strengthening of the assertion
       expect(error.message).toContain('Standalone binary notarization subprocess failed')
       expect(error.message).not.toContain('app-password')
+      expect(error.message).not.toContain('release@example.com')
       expect(error.message).toContain('[REDACTED]')

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 12372744-d05f-4d30-8e07-8450497be5e6

📥 Commits

Reviewing files that changed from the base of the PR and between 7f1e280 and 5753df9.

📒 Files selected for processing (2)
  • config/electron-builder.config.cjs
  • config/scripts/electron-builder-config.test.mjs

Comment thread config/electron-builder.config.cjs
brennanb2025 and others added 2 commits July 1, 2026 23:18
Co-authored-by: Orca <help@stably.ai>
Co-authored-by: Orca <help@stably.ai>
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.

[Bug]: macOS 1.4.106: nested simcam dylib is unnotarized and syspolicyd reports Malware rejection

1 participant