Skip to content

fix(ios): decrypt pushes on release builds by sharing state via the keychain - #14039

Merged
azooz2003-bit merged 2 commits into
mainfrom
feat-push-nse-keychain
Sep 23, 2026
Merged

azooz2003-bit merged 2 commits into
mainfrom
feat-push-nse-keychain

Conversation

@azooz2003-bit

@azooz2003-bit azooz2003-bit commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Since encrypted pushes landed in #12384 (2026-09-17), every push on INTERNAL, BETA, and App Store builds shows the placeholder "cmux / An agent needs your attention" instead of the agent's title and body. API-key-signed dev phone builds have shown the same since #13492 (2026-09-21).

The notification service extension decrypts each push. To do that it needs the signed-in account ID and the Mac key pinned during pairing. The host app wrote both to the group.dev.cmux.ios App Group suite. The host App IDs for those lanes do not have App Groups: in App Store Connect, dev.cmux.app.internal, com.cmux.app, and dev.cmux.app.beta list no APP_GROUPS capability, and only their .NotificationService App IDs do. cmux-release.entitlements also omits the group. An unentitled UserDefaults(suiteName:) writes to a private plist in the app's own container. The extension therefore found no account, returned empty content, and iOS fell back to the payload's generic alert. On the reporter's iPhone, INTERNAL's container holds Library/Preferences/group.dev.cmux.ios.plist with cmux.activeAccountID.dev.cmux.app.internal. The App Store app has the same private file.

The host app and its extension share the host keychain access group (TEAMID.<host bundle id>) in every signing lane, and the extension already reads the push private key from that group. iOS now stores the account marker and Mac key pins there as keychain items, behind a small PhonePushSharedStateStorage protocol. The Mac keeps process-local UserDefaults and still reads pins written in the old registry format. The extension also logs which check failed (account_mismatch, sender_not_pinned, decrypt_failed, and so on). Until now every failure was silent.

After upgrading, the host rewrites the account marker at launch and re-pins the Mac key on its next attach. No manual migration is needed. #13741 (2026-09-22) is unrelated to the storage bug. It fans each push out to every paired app, which increased the number of placeholder banners.

Testing

  • swift test in Packages/macOS/CmuxPhonePush: 9 tests pass. The new PhonePushSharedStateTests cover these cases:
    • The Mac encrypts a real notify request. Host-side stores built from a host bundle write the account and pin. Extension-side stores built from an extension bundle (with CMUXHostBundleIdentifier) read them and decrypt the title, body, and expiry.
    • Clearing the account on the host also clears it for the extension.
    • Pins evict the oldest entry past the 128-entry cap.
    • Mac pins in the old string-array registry format still load.
    • Keychain access groups are resolved correctly.
  • The package compiles for iOS (swift build --triple arm64-apple-ios17.0). ios/NotificationService/NotificationService.swift type-checks against it.
  • No red/green regression pair. A package test on macOS cannot reproduce the actual defect, which depends on per-target entitlements. The end-to-end proof is on-device: a push that shows real content on a release-lane or API-key-signed build.

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Fixes encrypted push notifications on INTERNAL, BETA, App Store, and API-key-signed dev builds showing the generic "An agent needs your attention" placeholder instead of the real title and body. The notification service extension needed the account ID and Mac key pins that the host app wrote to an App Group suite, but those signing lanes lack App Groups capabilities, so the extension found no account and iOS fell back to the payload's generic alert.

  • Stores the account marker and Mac key pins as keychain items in the host keychain access group, which both the host app and its extension already share in every signing lane, behind a PhonePushSharedStateStorage protocol exposed as a Bundle property.
  • The Mac keeps process-local UserDefaults and still reads pins written in the old registry format.
  • The extension now logs which check failed (account_mismatch, sender_not_pinned, decrypt_failed, expired) instead of failing silently.
  • No manual migration needed; the host rewrites the account marker at launch and re-pins the Mac key on its next attach.

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

Review in cubic

Summary by CodeRabbit

  • New Features

    • Push state can now be shared between the host app and notification extensions, with secure keychain storage on iOS.
    • Existing saved peer pins remain readable, and removing the active account clears it from shared storage.
  • Bug Fixes

    • Encrypted push notifications are checked separately for account match and trusted sender before decryption.
    • Suppressed notifications now include a reason, such as unavailable keys, decryption failure, or timeout.

Encrypted pushes (#12384) are opened by the notification service
extension, which needs the active account and the pinned Mac key. The
host app wrote both to the group.dev.cmux.ios App Group suite, but the
release host App IDs (INTERNAL, BETA, App Store) never had App Groups and
API-key-signed dev builds drop it (#13492). Their writes landed in a
private per-app plist, the extension found no account, and iOS showed the
generic "An agent needs your attention" alert for every push.

Both targets already share the host keychain access group in every
signing lane, so iOS now stores this state as keychain items there. The
Mac keeps its process-local defaults and reads its existing pins. The
extension logs which check failed instead of failing silently.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@coderabbitai

coderabbitai Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

Next included review available in 4 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: e2ef3768-9d8a-4e29-826d-aea27bb7a63a

📥 Commits

Reviewing files that changed from the base of the PR and between adcc7e0 and e0a97fa.

📒 Files selected for processing (2)
  • Packages/macOS/CmuxPhonePush/Sources/CmuxPhonePush/PhonePushCrypto.swift
  • Packages/macOS/CmuxPhonePush/Sources/CmuxPhonePush/PhonePushSharedStateStorage.swift

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: fbfdfa41-72bb-4313-a1b4-dc18672d19cd

📥 Commits

Reviewing files that changed from the base of the PR and between 1963a38 and adcc7e0.

📒 Files selected for processing (4)
  • Packages/macOS/CmuxPhonePush/Sources/CmuxPhonePush/PhonePushCrypto.swift
  • Packages/macOS/CmuxPhonePush/Sources/CmuxPhonePush/PhonePushSharedStateStorage.swift
  • Packages/macOS/CmuxPhonePush/Tests/CmuxPhonePushTests/PhonePushSharedStateTests.swift
  • ios/NotificationService/NotificationService.swift

Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

The change adds a shared storage abstraction for push account and peer-key state, with platform-specific backends. It also adds reason-specific logging for suppressed notifications in the notification service.

Changes

Shared push state

Layer / File(s) Summary
Storage contract and platform adapters
Packages/macOS/CmuxPhonePush/Sources/CmuxPhonePush/PhonePushSharedStateStorage.swift
A storage protocol provides data reads, writes, removals, and prefix-based key lookup. Default storage uses Keychain on iOS and UserDefaults elsewhere. The UserDefaults adapter also reads legacy string-array values.
Peer and account stores use shared storage
Packages/macOS/CmuxPhonePush/Sources/CmuxPhonePush/PhonePushCrypto.swift
The peer-key and active-account stores use the storage protocol. They write and remove values through the protocol, and the peer registry retains its 128-entry limit.
Host and extension state validation
Packages/macOS/CmuxPhonePush/Tests/CmuxPhonePushTests/PhonePushSharedStateTests.swift
Tests cover host-to-extension state access, account clearing, peer eviction, legacy registry compatibility, and access-group parsing.

Notification suppression logging

Layer / File(s) Summary
Reason-specific suppression logging
ios/NotificationService/NotificationService.swift
Suppression paths pass distinct reasons to finishSuppressed. The function logs the reason before clearing notification content and finishing.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant HostApp
  participant SharedStateStorage
  participant NotificationService
  HostApp->>SharedStateStorage: Store active account and pinned peer key
  NotificationService->>SharedStateStorage: Read active account and pinned peer key
  NotificationService->>NotificationService: Decrypt notification payload
Loading

Merge Risk: ⚪ Minimal · up to adcc7

The supplied evidence does not establish a reason to block merging. Release-build push decryption still needs on-device validation.


Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error, 1 warning)

Check name Status Explanation Resolution
Cmux Swift Actor Isolation ❌ Error The production diff adds the file-scoped notificationServiceLog constant without nonisolated in ios/NotificationService/NotificationService.swift:6. The repository rule requires file-scoped `Log… Declare the logger as nonisolated private let notificationServiceLog = Logger(...). Keep the logger outside MainActor isolation so notification-service callbacks can use it without actor-isolation coupling.
Docstring Coverage ⚠️ Warning Docstring coverage is 13.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 30 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (23 passed)
Check name Status Explanation
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.
Cmux Cloud Persistent Session And Early Input ✅ Passed PASS: The PR changes iOS push-state storage, notification decryption guards, and related tests. The authoritative diff contains no Cloud terminal creation, cmux-tui transport, manual renderer, Ghostty…
Cmux Swift Blocking Runtime ✅ Passed The production Swift diff introduces no semaphore, blocking wait, sleep, delayed dispatch, timer, polling loop, main-queue sync, or new manual lock. The existing NSLock declarations and withLock h…
Cmux Browser Automation Off-Main ✅ Passed PASS: The PR changes only phone-push storage, tests, and notification-service logging. The authoritative diff contains no browser.* commands, WebKit/AppKit waits, socket-worker routing, or browser p…
Cmux Expensive Synchronous Load ✅ Passed PASS. The PR changes push-state storage and notification-service diagnostics only. The changed production files add no RestorableAgentSessionIndex, SharedLiveAgentIndex, agent hook/session stores,…
Cmux Cache Substitution Correctness ✅ Passed PASS: The diff does not replace an authoritative read with a cache. PhonePushPeerKeyStore and PhonePushActiveAccountStore now call the injected storage on each read, while iOS storage calls `SecIt…
Cmux No Hacky Sleeps ✅ Passed PASS. The authoritative PR range changes only Swift files: three *.swift files in CmuxPhonePush and one Swift notification-service file. The rule applies to TypeScript, JavaScript, shell, and non-…
Cmux Algorithmic Complexity ✅ Passed No algorithmic-complexity failure is introduced. The peer registry remains explicitly capped at 128 entries, and its existing discoveredKeys plus orderedKeys.contains logic is unchanged from the b…
Cmux Swift Concurrency ✅ Passed The pull request adds no prohibited async patterns. The changed production Swift files contain no new DispatchQueue/DispatchGroup, Combine, completion-handler, async/await, or fire-and-forget Task usa…
Cmux Swift @Concurrent ✅ Passed The changed Swift code introduces no async, nonisolated async, @concurrent, Task, or actor-isolated declarations. The new keychain and UserDefaults storage methods, store methods, tests, and n…
Cmux Swift Package Boundaries ✅ Passed The production feature logic remains behind the existing CmuxPhonePush SwiftPM target. The diff adds PhonePushSharedStateStorage, keychain/UserDefaults persistence, and store changes under `Packag…
Cmux Swiftpm Lockfiles ✅ Passed PASS. The pull request changes only four Swift source/test files. It does not change any Package.swift, Package.resolved, Xcode project/workspace, .gitignore, workflow, or dependency metadata file. Pa…
Cmux Swift Logging ✅ Passed The diff adds one production diagnostic in ios/NotificationService/NotificationService.swift: an OSLog.Logger error with a fixed suppression reason. It does not add print, debugPrint, dump, …
Cmux User-Facing Error Privacy ✅ Passed PASS — The production diff adds only internal OSLog diagnostics in the notification service. The reason values are logged through Logger.error and are not inserted into notification content or ano…
Cmux Full Internationalization ✅ Passed PASS: The production diff introduces no new user-facing text. The added Swift literals are keychain/configuration identifiers, protocol reason tokens, tests, or an OSLog debug message. The notificatio…
Cmux Swiftui State Layout ✅ Passed PASS: The PR does not change SwiftUI views or SwiftUI observation/layout state. The authoritative diff only changes push crypto storage, notification-service logic, and tests. No added `ObservableObje…
Cmux Architecture Rethink ✅ Passed The change adds a documented keychain bridge for state that must be shared between the iOS host app and notification extension. PhonePushSharedStateStorage is the single storage abstraction, with ke…
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PASS. The pull request changes push-state storage, crypto handling, tests, and notification-service logging. The authoritative diff adds or changes no NSWindow, NSPanel, NSWindowController, SwiftUI Wi…
Cmux Source Artifacts ✅ Passed All four changed paths are intentional Swift source or test files: the storage implementation, crypto-store updates, notification-service logging, and shared-state tests. The diff adds no logs, screen…
Cmux No Test Or Debug Seam In Production Source ✅ Passed PASS. The only changed Swift files under production Sources/ are PhonePushCrypto.swift and the new PhonePushSharedStateStorage.swift. The diff adds no #if DEBUG, test-build guard, or member na…
Title check ✅ Passed The title clearly and concisely identifies the main change: fixing iOS push decryption on release builds by sharing state through the keychain.
Description check ✅ Passed The description clearly explains the problem, root cause, implementation, compatibility behavior, logging changes, and testing. It does not include the template's Demo Video, Review Trigger, or Checkl…
Full details: Cmux Swift Actor Isolation

Explanation

The production diff adds the file-scoped notificationServiceLog constant without nonisolated in ios/NotificationService/NotificationService.swift:6. The repository rule requires file-scoped Logger constants to opt out of unnecessary MainActor coupling. The logger is used by notification-service callback code, so this is a changed isolation issue rather than existing debt. The new storage protocol has only synchronous requirements, and the changed storage structs are not Sendable, so no separate failure applies.

✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

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.

The namespace lint rejects an all-static enum; the default storage is
derived from the bundle's Info.plist, so it reads as a Bundle property.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@azooz2003-bit
azooz2003-bit merged commit 1ba90a1 into main Sep 23, 2026
53 checks passed
rustybret pushed a commit to rustybret/bmux that referenced this pull request Sep 24, 2026
1ba90a1 fix(ios): decrypt pushes on release builds by sharing state via the keychain (manaflow-ai#14039)
73f12e5 Regenerate config schema and shortcut docs for toggleFileEditorWordWrap (manaflow-ai#14052)
4dafd99 Keep startup retry in reconnecting state (manaflow-ai#13856)
58bbcfa coderouter: report Server-Timing on every route (manaflow-ai#13976)
e5a1d11 Drop the retired staging legacy Subrouter default (manaflow-ai#13946)
727d3f0 ci: fetch previous nightly DMGs by asset id and treat misses as no delta (manaflow-ai#13970)
72490a9 ci: make cross-run product reuse actually adopt products (manaflow-ai#14007)

# Conflicts:
#	.github/workflows/ci-macos.yml
#	.github/workflows/persistent-macos-compile.yml
#	.github/workflows/test-e2e.yml
#	.github/workflows/test-ios.yml
azooz2003-bit added a commit that referenced this pull request Sep 24, 2026
#14039 moved the account marker the notification extension reads into
the shared keychain, but the host only wrote it on sign-in or after a
protected-data unlock. A launch that restores a cached session never
wrote it, so an updated release build kept showing "An agent needs your
attention" until the user signed in again.

The auth composition now mirrors authenticatedSessionIdentities() into
the store for the app's lifetime. The stream yields the current identity
first, so a restored session writes the marker immediately, and later
sign-in, account switches, and sign-out follow through the same path.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@azooz2003-bit

Copy link
Copy Markdown
Collaborator Author

Correction: the description says the host rewrites the account marker at launch. It does not. The host only wrote it on sign-in or after a protected-data unlock, so after updating, pushes stayed on the placeholder until the next sign-in. Follow-up: #14110

azooz2003-bit added a commit that referenced this pull request Sep 24, 2026
…#14110)

* fix(ios): mirror the signed-in account for push decryption at launch

#14039 moved the account marker the notification extension reads into
the shared keychain, but the host only wrote it on sign-in or after a
protected-data unlock. A launch that restores a cached session never
wrote it, so an updated release build kept showing "An agent needs your
attention" until the user signed in again.

The auth composition now mirrors authenticatedSessionIdentities() into
the store for the app's lifetime. The stream yields the current identity
first, so a restored session writes the marker immediately, and later
sign-in, account switches, and sign-out follow through the same path.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

* fix: import workspace liveness in Codex restore policy

* fix: declare the restore observation before the Codex intent check

36c3050 used matchingObservation in the Codex restore-intent check
one line before declaring it, so main stopped compiling ("use of local
variable 'matchingObservation' before its declaration"). Declare the
observation first; the check order and inputs are unchanged.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
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.

1 participant