Repository navigation
feat: [SDK-5566] install the iOS SDK from OneSignal-XCFramework with the NSE and App Group - #83
sherwinski wants to merge 6 commits into
Conversation
abdulraqeeb33
left a comment
abdulraqeeb33
left a comment
There was a problem hiding this comment.
Reviewed against the PR head. Note the baseline: this PR is stacked on #82 (sherwin/sdk-5562), so the file-reading passes treated #82's APNs Bundle ID work as existing baseline.
The shape of this is good, and the product-to-target map, the resolver output, and the bundle/link hygiene all check out. Six comments inline. Two things land on lines outside the diff, so they get no anchor:
skills/setup/SKILL.md:306 — Step 5 updated the allow-list bullet to include the OneSignalNotificationServiceExtension/ folder, but ten lines down it still reads "Minimal integration only: SDK init in the correct lifecycle spot plus what the verification helper needs — no extra OneSignal features unless the user asked (safety contract §8)." §8 as amended by this PR now says the opposite for this platform: "On native iOS the minimal integration includes the Notification Service Extension, the App Group, and the OneSignalInAppMessages product." Step 5 is the step that computes the diff and takes the one approval, so the sentence in front of the agent at write time tells it to leave out the three things this PR exists to add. AGENT.md's rule is to change the contract first and then the skills; the contract moved and this line did not.
The iOS setup eval scenario is recorded as not run. AGENT.md: "If a change affects setup, credentials, or verify behavior, run the covering scenarios before merge and record the verdicts and session IDs in the PR. Do not trust a skill edit on read-through alone." The plutil -lint and compile gates cover template syntax, but nothing exercised the pbxproj recipe end to end, and that recipe is the riskiest thing here. Not asking for it before review, but it should land before merge.
Smaller things, grouped so they do not each need a thread:
scripts/verify_integration.py:399— theNotificationService.swiftleg only requires the substringOneSignalExtension, so a file containing just the import passes. The siblingios_verification_uses_push_observerdemands concrete symbols.- §8 now makes
OneSignalInAppMessagespart of the minimal install, but there is no check for it, while the NSE added in the same change did get one. scripts/package_directory_bundle.py:224—fenced_code_linestoggles on any line starting with triple backticks and never checks pairing, so one unclosed fence exempts the rest of the file from the../gate.skills/setup/ios-nse.md:141—GENERATE_INFOPLIST_FILE = YESalongsideINFOPLIST_FILEpointing at a real file. The troubleshooting table in the same file lists "Multiple commands produce Info.plist" and attributes it to synchronized-group membership; is the build-setting pair definitely not a second cause here?skills/setup/ios-nse.md:226— step 7 says to build after a CocoaPods install, which needs the.xcworkspace, but the precedingxcodebuild -listnames-project <App>.xcodeprojand the build step does not say which.scripts/resolve_sdk_version.py:46— the block comment's "OneSignal-XCFramework ... publishes the same version tags, so the pin applies to both" is the same sentence as thenotestring six lines below. One fact, two copies, in one file.
| if os.path.basename(fp) in ("project.pbxproj", "Podfile")] | ||
| blob = "\n".join(read(fp) for fp in manifests) | ||
| target = "OneSignalNotificationServiceExtension" in blob | ||
| linked = re.search(r"productName\s*=\s*OneSignalExtension|pod\s+['\"]OneSignal", blob) is not None |
There was a problem hiding this comment.
This passes unconditionally on CocoaPods. The alternation's second branch is pod\s+['\"]OneSignal, and the Podfile this PR generates puts pod 'OneSignal/OneSignal', '{v}' on the app target (resolve_sdk_version.py podfile mode). So linked is true for every CocoaPods install the skill produces, whether or not an NSE exists, and the "not linked to the extension" detail can never fire on that path.
The sibling you added in the same change scopes it properly: pod\s+['\"]OneSignal/OneSignalLocation. The trouble is that the same tightening is not available here, because podfile-nse emits the identical line as the app target, pod 'OneSignal/OneSignal'. So no regex over the whole blob can tell the two apart; the check has to scope to the target 'OneSignalNotificationServiceExtension' do block before looking for the pod. Same for the SPM branch, where productName = OneSignalExtension is matched anywhere in the file rather than on the extension's target.
There was a problem hiding this comment.
Fixed in 72945a1. On CocoaPods, the check reads only the target 'OneSignalNotificationServiceExtension' do … end block. On SPM, it follows the NSE target's packageProductDependencies to a product with productName = OneSignalExtension. A product or pod on the app target no longer passes. I tested 7 fixture projects and one real Xcode project (the Cordova demo app).
| - `requestPermission` on iOS DOES take a completion block: `OneSignal.Notifications.requestPermission({ accepted in ... }, fallbackToSettings: false)` (unlike Android's suspend form). | ||
|
|
||
| The Step-8 structural self-check (`verify_integration.py --platform ios`) enforces `#if DEBUG`, the real push observer (not NotificationCenter), and init in a launch context. Non-negotiable properties (SKILL.md Step 6): | ||
| The Step-8 structural self-check (`verify_integration.py --platform ios`) enforces `#if DEBUG`, the real push observer (not NotificationCenter), init in a launch context, the NSE target with `OneSignalExtension` linked, one App Group shared by the app and extension entitlements, and, when `OneSignalLocation` is linked, a location usage description. Non-negotiable properties (SKILL.md Step 6): |
There was a problem hiding this comment.
"Enforces" and "non-negotiable properties" overstate what Step 8 does for three of the six items listed. ios_nse_present is warn. ios_app_group_matches is warn when either entitlements file has no group at all, and only error on a genuine mismatch. ios_location_usage_description is warn. Only the push observer in that list is error.
So a tree with no NSE target, no NotificationService.swift, and no App Group anywhere still gets verdict: pass. The PR description says the warn levels are deliberate, which is fine, but then this sentence should say Step 8 reports on those rather than enforces them, otherwise an agent treats a passing verdict as proof the NSE landed.
There was a problem hiding this comment.
Fixed in c0067f5. The text now names the 3 error-level checks and lists the others as warnings. setup.platform_config ok now needs the self-check to pass ios_nse_present and ios_app_group_matches. I changed the telemetry contract first.
| ```xml | ||
| <key>com.apple.security.application-groups</key> | ||
| <array> | ||
| <string>group.MAIN_APP_BUNDLE_ID.onesignal</string> |
There was a problem hiding this comment.
The bundle ID placeholder has three spellings across this flow, and an agent doing a literal substitution will only catch one of them. The template file and compile_check_ios.sh use __MAIN_APP_BUNDLE_ID__, and the table at line 31 matches. But line 9 and ios.md:105 write <MAIN_APP_BUNDLE_ID>, and this line plus line 48 and line 150 use bare MAIN_APP_BUNDLE_ID.
Line 150 is the one that bites: PRODUCT_BUNDLE_IDENTIFIER = MAIN_APP_BUNDLE_ID.OneSignalNotificationServiceExtension; goes straight into the pbxproj, so a missed substitution ships a literal MAIN_APP_BUNDLE_ID.... as the extension's bundle identifier and signing fails. Worth picking one spelling for all of them.
There was a problem hiding this comment.
Fixed in c0067f5. Every file now uses __MAIN_APP_BUNDLE_ID__, the same as the template and compile_check_ios.sh.
|
|
||
| Prefer the text edit: the user's host usually has no Xcode UI, and the objects below are what File ▸ New ▸ Target writes. Preview them in the Step-5 diff like every other pbxproj edit (safety contract §5). If the pbxproj format does not match the shapes below, stop the edit and hand the target creation to the human (File ▸ New ▸ Target ▸ Notification Service Extension, named `OneSignalNotificationServiceExtension`); then still write the 3 files, the App Group, and the product link, and report `setup.platform_config fail capability_manual` ([ios.md](ios.md) → Checkpoint). | ||
|
|
||
| Every object needs a unique 24-character hex ID. The placeholders `NSE0000000000000000000X` below stand for IDs you generate; check that none collides with an existing ID. Replace `MAIN_APP_BUNDLE_ID`, `<PROJECT_OBJECT_ID>`, and the deployment target with the project's values. |
There was a problem hiding this comment.
This says to check that no generated hex ID collides with an existing one, but nothing says to check whether an OneSignalNotificationServiceExtension target already exists before adding one. Safety contract §2 is "Detect prior installation FIRST ... Found → propose update/repair, never a duplicate", and §6's idempotency keys off the onesignal:managed marker, which pbxproj objects do not carry.
So a repair run over a project that already has an NSE appends a second target with fresh IDs, and §6 cannot catch it the way it catches template rewrites. A precondition check at the top of this section would close it.
There was a problem hiding this comment.
Fixed in c0067f5. The new step 0 says: if a PBXNativeTarget named OneSignalNotificationServiceExtension exists, repair it, and put only the missing parts in the diff (safety contract §2).
| @@ -0,0 +1,10 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
This template and the Info.plist one next to it have no onesignal:managed v1 marker, while every sibling template does: NotificationService.swift.tmpl line 1, OneSignalManager.swift.tmpl, OneSignalSetupVerification.swift.tmpl. Safety contract §6 is "Mark generated blocks: // onesignal:managed v1 — idempotency keys off this marker on re-runs", and detect_platform.py's prior_onesignal greps for exactly that string.
A plist comment works fine for this. Without it these two files are invisible to prior-install detection.
There was a problem hiding this comment.
Fixed in c0067f5. Both templates have <!-- onesignal:managed v1 --> after the XML declaration. compile_check_ios.sh still accepts both files as valid plists.
| pod 'OneSignal/OneSignal', 'X.Y.Z' # onesignal:managed v1 | ||
| end | ||
| ``` | ||
| The `OneSignal` pod vendors the same XCFrameworks as the SPM package, so CocoaPods needs no repository change. CocoaPods 1.16.2+ is required; on an Xcode 16 project, `Unable to find compatibility version string for object version '70'` means the `xcodeproj` gem is too old — the documented workaround is to set `objectVersion = 55;` in the pbxproj before `pod install`. |
There was a problem hiding this comment.
Setting objectVersion = 55 downgrades the project file format on an Xcode 16 project, and it is mentioned in passing as "the documented workaround" rather than as a gated step. Everything else this PR writes to the pbxproj goes through the §5 diff preview and has a capability_manual fallback; this one has neither, no idempotency marker, and no stated revert if pod install or the next Xcode open goes wrong.
Should this be a conditional step the agent only takes after pod install actually fails with that error, and include it in the previewed diff?
There was a problem hiding this comment.
Fixed in c0067f5. The edit now happens only after that exact pod install error. The skill first suggests a CocoaPods update. If the user cannot update, it shows the line as its own diff with the revert, and asks for approval.
abdulraqeeb33
left a comment
abdulraqeeb33
left a comment
There was a problem hiding this comment.
Follow-up pass, this time reviewing #82 and #83 as the combined state that lands on main rather than each diff on its own. One finding that neither single-PR review could reach, plus one observation. Nothing here changes the verdict already on this PR.
The project's Bundle ID becomes ambiguous once the NSE exists
This PR gives every native iOS project a second target carrying its own identifier:
PRODUCT_BUNDLE_IDENTIFIER = MAIN_APP_BUNDLE_ID.OneSignalNotificationServiceExtension;
(skills/setup/ios-nse.md:150)
You clearly had the hazard in mind inside this PR, because ios-nse.md:16 says: "Build the group name from the main app target's Bundle ID, never the extension's."
#82 adds two instructions to read "the project's Bundle ID" and neither carries that qualifier:
skills/credentials/SKILL.md:106— "Read the project's Bundle ID the way the .p8 flow does (the Xcode project first) and compare it withapns.bundle_id."skills/setup/SKILL.md:249— "compareplatforms.apns.bundle_id(theapns_bundle_idfield) with the project's Bundle ID."
Nor does the .p8 read they point at, skills/credentials/SKILL.md:175.
Each file is right on its own. #82's text is correct against today's main, where an iOS project has one bundle ID. This PR's text is correct in isolation, because it guards the confusion inside its own file. Together, #82's reads turn ambiguous on exactly the projects this PR produces. Three consequences, worst last:
- The
.p8flow's "returns more than one candidate → ask the user" branch goes from a rare fallback to the default on any already-integrated project. - If the agent picks the extension's identifier, #82's new comparison reports a false mismatch and tells the user "APNs will reject every push to this app", because
<app>.OneSignalNotificationServiceExtensioncan never equal the storedapns_bundle_id. - On the
.p8upload path the Bundle ID is one of four parameters to the write-once endpoint.credentials/SKILL.md:175says "Never guess it: all four params are required, and the endpoint is write-once." A wrong value there is not recoverable through the API.
On reachability, so this is not overstated: a single fresh run is safe, because credentials closes at Step 3 before the install writes the NSE at Step 5. What is exposed is a repair or re-run over an already-integrated project, which safety contract §2 explicitly supports, and the credentials skill invoked standalone.
Cheapest fix is probably to carry the ios-nse.md:16 qualifier into those reads, or to have the read skip any target whose identifier ends in .OneSignalNotificationServiceExtension. The edit lands in #82's files but the hazard arrives with this one, so it seemed more useful here than as a late comment on an approved PR.
Two green funnel rows get weaker in the same release
Not a defect, just something only visible across the stack. #82 widens credentials.detected ok to cover a user's dashboard self-report where it previously recorded unknown. This PR widens setup.platform_config ok to assert an NSE and App Group that Step 8 only checks at warn and that can fall back to a human hand-off. Each is reasonable alone. Shipping both at once means two consecutive milestones become weaker evidence in the same release, so a real drop in iOS onboarding success would be harder to spot. Worth being deliberate about rather than letting it fall out.
Version
Separately, I got this wrong in my review on #82 and have corrected it there: the combined change set does need a version bump. 1.2.3 is the latest shipped release tag and main's most recent version commit is chore: Release 1.2.3, so AGENT.md's "already moved past the last release" carve-out does not apply.
A pbxproj snippet in a skill carries an rpath with ../../ in it. That text goes into the user's project and is not a link the plugin resolves, so the gate now skips fenced code blocks for the parent-segment check only. A new self-test case keeps a ../ in prose rejected.
… App Group The resolver's SPM line now names the OneSignal-XCFramework package and the product-to-target map; the Podfile output gains the in-app messages subspec and a line for the extension target. The default product set is OneSignalFramework and OneSignalInAppMessages on the app target and OneSignalExtension on the extension; OneSignalLocation is added only on request. The Notification Service Extension and the App Group are now part of the default native iOS install. ios-nse.md holds the recipe, 3 new templates hold the extension files, compile_check_ios.sh compiles the Swift one against a stub of OneSignalExtension.h, and verify_integration.py checks the target, the link, and the shared App Group. The safety and telemetry contracts, the platform matrix, and the prior-install detection follow.
The documentation marks OneSignalLocation as optional, so setup now asks one structured question before the Step-5 diff and adds the module only on a yes. The question is skipped when the request already answers it or a prior install links the module. On a yes, the change set gains the product (or the new podfile-location resolver line), NSLocationWhenInUseUsageDescription, and OneSignal.Location.isShared = true after initialize, because the SDK defaults sharing to off. Setup still adds no location prompt at launch; the summary names the in-app message prompt and requestPermission(). verify_integration.py warns when OneSignalLocation is linked without a location usage description.
The link check now reads only the NSE target: its packageProductDependencies on SPM, or its target block in the Podfile. A OneSignal product on the app target no longer passes it. The service check needs the didReceiveNotificationExtensionRequest call, and a new warn check reports a missing OneSignalInAppMessages link.
A fence that never closes no longer exempts the rest of the file. The self-test covers the case.
- Use one Bundle ID placeholder, __MAIN_APP_BUNDLE_ID__, in every file. - Repair an existing NSE target instead of adding a second one. - Add the onesignal:managed v1 marker to the 2 plist templates. - Name the native iOS exception to minimal integration in Step 5. - Report setup.platform_config ok only when the self-check passes the NSE and App Group rows. - Make the objectVersion = 55 workaround a separate, approved edit after the pod install error. - Build the workspace after pod install.
e078601 to
c0067f5
Compare
|
Replies to the items in the 2 review bodies. The inline threads have their own replies. First review:
Combined review:
|
|
|
||
| ### 0. Check for an existing target first | ||
|
|
||
| pbxproj objects cannot hold the `onesignal:managed v1` marker, so a second run cannot find its own objects by the marker. Before you add anything, search `project.pbxproj` for a `PBXNativeTarget` whose `name` is `OneSignalNotificationServiceExtension` (or any target with `productType = "com.apple.product-type.app-extension"` that already has a `NotificationService` class calling `OneSignalExtension`). If one exists, do not add the objects below (safety contract §2): repair the existing target instead. Compare it with the "Required outcomes" list, and put only the missing parts in the Step-5 diff (the product link, the App Group, the embed phase, or the 3 files). |
There was a problem hiding this comment.
Step 0 covers one case: an existing extension that already calls OneSignalExtension. Two others fall through.
An app coming from Braze, Airship, or FCM rich push has an NSE that doesn't call it, so the recipe adds a second one. iOS runs one per app and the loser fails silently.
An existing OneSignal NSE under another name gets repaired but can never pass ios_nse_present or ios_app_group_matches, which match the literal name only (verify_integration.py:114, :463, :492), so the checkpoint records capability_manual for a hand-off that didn't happen.
I'd treat any pre-existing NSE as a stop-and-ask, and have the verifier find the target by productType plus the OneSignalExtension link.
|
|
||
| ## Add the NSE target to `project.pbxproj` | ||
|
|
||
| Prefer the text edit: the user's host usually has no Xcode UI, and the objects below are what File ▸ New ▸ Target writes. Preview them in the Step-5 diff like every other pbxproj edit (safety contract §5). If the pbxproj format does not match the shapes below, stop the edit and hand the target creation to the human (File ▸ New ▸ Target ▸ Notification Service Extension, named `OneSignalNotificationServiceExtension`); then still write the 3 files, the App Group, and the product link, and report `setup.platform_config fail capability_manual` ([ios.md](ios.md) → Checkpoint). |
There was a problem hiding this comment.
"Still write the 3 files, the App Group, and the product link" can't run once target creation went to the human. The SPM link lives on a target that doesn't exist yet, and on CocoaPods step 6 runs pod install, which fails on a Podfile target with no Xcode target. Say what to defer, and give the human a resume step.
|
|
||
| ### 4. Build configurations | ||
|
|
||
| Copy `IPHONEOS_DEPLOYMENT_TARGET`, `SWIFT_VERSION`, `DEVELOPMENT_TEAM`, and the signing style from the main app target. Debug and Release are identical except for `name`. |
There was a problem hiding this comment.
This says to copy the signing style, then line 143 hardcodes CODE_SIGN_STYLE = Automatic.
MARKETING_VERSION and CURRENT_PROJECT_VERSION aren't in the copy list either. They're 1.0/1 here and as literals in the Info.plist template, so every app past 1.0 gets ITMS-90473/90474 on each upload, and bumping the build setting later won't change the plist.
One-Line Summary
Native iOS setup installs the SDK from
OneSignal-XCFrameworkwith the documented product-to-target map, and creates the Notification Service Extension and App Group by default.Ticket: SDK-5566
Details
The resolver's SPM line now names
OneSignal-XCFramework(the bug-bash run pinnedOneSignal-iOS-SDK, the source repository) withOneSignalFramework+OneSignalInAppMessageson the app target andOneSignalExtensionon the extension; the Podfile output matches the documentation's 2 target blocks.ios.mdmakes the NSE and App Group part of the default install, with the recipe in the newios-nse.mdand 3 new templates;verify_integration.pychecks the target, the link, and the shared App Group. The safety contract §8, the telemetry contract, and the platform matrix say the same.Notes
OneSignalInAppMessagesis in by default (the documentation marks it Recommended, and it needs no code).OneSignalLocationis optional: setup asks one structured question before the diff (skipped when the request already answers it or a prior install links it). A yes adds the product or pod line,NSLocationWhenInUseUsageDescription, andOneSignal.Location.isShared = true(the SDK defaults sharing to off), but no location prompt at launch;verify_integration.pywarns when the module is linked without a usage description.OneSignal-iOS-SDK;OneSignal-XCFrameworkpublishes the same tags, so the pin applies to both (checked: tags 5.5.1 to 5.8.0 exist in both). The "Add XCFramework channels to sdk-releases" item from the ticket is a change in the sdk-releases repository and is not in this PR.capability_manualclass.../check, because the rpath in the pbxproj snippet needs../../. A self-test case keeps a../in prose rejected.cross-platform.md) keep the NSE optional; this PR covers native iOS only.Testing
compile_check_ios.sh(3 templates compile or lint; 3 negative controls rejected)verify_integration.py --platform ioson a scratch fixture: good tree passes; missingNotificationService.swift→ warn; App Group mismatch → errorresolve_sdk_version.py ios(json, podfile, podfile-nse)package_directory_bundle.py --checkand--self-testAffected areas
skills/setup/SKILL.mdskills/setup/assets/skills/credentials/SKILL.mdskills/verify/SKILL.mdscripts/references/