Repository navigation
feat: [SDK-5562] check the stored APNs Bundle ID and guide the no-key case - #82
sherwinski wants to merge 3 commits into
Conversation
The view-app read already returns apns_bundle_id, the Bundle ID that OneSignal sends to APNs as the topic. The script now prints it under platforms.apns.bundle_id, and the credentials and setup skills compare it with the project's Bundle ID when APNs is configured. A mismatch is the one stored-credential fault visible before a test send. When the presence check cannot run, the credentials skill now sends the Push Platforms URL and asks the user what the dashboard shows, instead of a portal walkthrough that may create a key the team already has. The .p8 flow says that a key is team-level, that Apple allows 2 per team, and which key scopes do not cover this app.
abdulraqeeb33
left a comment
abdulraqeeb33
left a comment
There was a problem hiding this comment.
Reviewed the four changed files with a ten-lens parallel pass against the PR head. The Bundle ID comparison and the no-key rewrite both read well. The findings below are all about the second copy of a rule rather than the rule itself.
Four comments inline. The setup-side guard gap is the one I would fix before merge; the rest are questions.
One minor note, not worth its own thread: item 1 describes apns.auth_type as "(p8 or p12)" and item 4 asks the agent to name it as "p8 key or p12 certificate", but _presence returns None when exactly one of apns_key_id / apns_team_id is set, which api-reference.md itself documents as "unknown (report null)". Narrow state, but neither sentence has a branch for it.
Checked and not raised: no version bump is needed, main is already at 1.2.3; the #apple-apns-p8-flow anchor resolves; the §14 two-or-three option limit is respected; and platforms.apns.bundle_id is documented consistently across api-reference, both skills, and the note string.
| The single worst onboarding failure is installing the SDK before push credentials exist: the app builds, the device registers, and it shows up **unsubscribed** because OneSignal has nothing to hand APNs/FCM. Close credentials FIRST. Never skip this step silently. | ||
|
|
||
| 1. **Check what's configured.** With an app-scoped key available (`$ONESIGNAL_SETUP_TOKEN` / `$ONESIGNAL_REST_API_KEY`), run `python3 <plugin>/scripts/onesignal_api.py app <APP_ID>` — the view-app read, `GET /api/v1/apps/{APP_ID}` (app auth works) — and read the verdict for the platform you're about to install: Android → `platforms.fcm.configured`, iOS → `platforms.apns.configured`, Web → `platforms.web.configured` (or the free `web-probe` below). The script decides from the **non-secret presence fields** only (`apns_env`, `fcm_sender_id`, `chrome_web_origin`; api-reference "View an app"), and treats `null`, `""`, and an absent key the same. Without Python (Step 0.2), `curl` the same `GET` with whichever key env var is set, and keep only the HTTP status, the app `id`, and the presence fields — the raw body still carries the plaintext credentials until the server removes them, so never print or paste the whole body: `curl -sS -w '\nHTTP %{http_code}\n' -H "Authorization: Key ${ONESIGNAL_REST_API_KEY:-$ONESIGNAL_SETUP_TOKEN}" "https://api.onesignal.com/api/v1/apps/<APP_ID>" | grep -oE '"(id|apns_env|apns_key_id|apns_team_id|fcm_sender_id|chrome_web_origin)":[^,}]*|^HTTP [0-9]+'`. A verdict exists **only** when the `HTTP` line is 2xx **and** the `"id"` line shows this App ID — that pair is your proof the body is the app object. Then read the presence fields and nothing else — never the plaintext credential fields (`apns_p8`, `fcm_v1_service_account_json`, `gcm_key`). Any other output (no `HTTP` line, a non-2xx status, no `"id"` line, a curl error) is **presence unknown, not "missing"**: do not start the credentials skill on it — take the no-key path below instead. In a sandboxed runtime, a script exit code of 3 or a curl error on the first try means the sandbox blocked the request: re-run that one command with escalation first ("Network access" above), and read presence unknown only if the re-run also fails or the user declines. No key available, or presence unknown → ask the user to check the dashboard and tell you. Send the page as a clickable URL built from the App ID: `https://dashboard.onesignal.com/apps/<APP_ID>/settings/push/platforms` (Settings > Push Platforms). | ||
| 1. **Check what's configured.** With an app-scoped key available (`$ONESIGNAL_SETUP_TOKEN` / `$ONESIGNAL_REST_API_KEY`), run `python3 <plugin>/scripts/onesignal_api.py app <APP_ID>` — the view-app read, `GET /api/v1/apps/{APP_ID}` (app auth works) — and read the verdict for the platform you're about to install: Android → `platforms.fcm.configured`, iOS → `platforms.apns.configured`, Web → `platforms.web.configured` (or the free `web-probe` below). The script decides from the **non-secret presence fields** only (`apns_env`, `fcm_sender_id`, `chrome_web_origin`; api-reference "View an app"), and treats `null`, `""`, and an absent key the same. Without Python (Step 0.2), `curl` the same `GET` with whichever key env var is set, and keep only the HTTP status, the app `id`, and the presence fields — the raw body still carries the plaintext credentials until the server removes them, so never print or paste the whole body: `curl -sS -w '\nHTTP %{http_code}\n' -H "Authorization: Key ${ONESIGNAL_REST_API_KEY:-$ONESIGNAL_SETUP_TOKEN}" "https://api.onesignal.com/api/v1/apps/<APP_ID>" | grep -oE '"(id|apns_env|apns_key_id|apns_team_id|apns_bundle_id|fcm_sender_id|chrome_web_origin)":[^,}]*|^HTTP [0-9]+'`. A verdict exists **only** when the `HTTP` line is 2xx **and** the `"id"` line shows this App ID — that pair is your proof the body is the app object. Then read the presence fields and nothing else — never the plaintext credential fields (`apns_p8`, `fcm_v1_service_account_json`, `gcm_key`). **iOS, when APNs is configured:** compare `platforms.apns.bundle_id` (the `apns_bundle_id` field) with the project's Bundle ID. A difference means APNs will reject every push to this app; say so in one sentence with both values and the dashboard URL below, and keep going (credentials skill Step 1, item 4, has the detail). Any other output (no `HTTP` line, a non-2xx status, no `"id"` line, a curl error) is **presence unknown, not "missing"**: do not start the credentials skill on it — take the no-key path below instead. In a sandboxed runtime, a script exit code of 3 or a curl error on the first try means the sandbox blocked the request: re-run that one command with escalation first ("Network access" above), and read presence unknown only if the re-run also fails or the user declines. No key available, or presence unknown → ask the user to check the dashboard and tell you. Send the page as a clickable URL built from the App ID: `https://dashboard.onesignal.com/apps/<APP_ID>/settings/push/platforms` (Settings > Push Platforms). |
There was a problem hiding this comment.
This copy of the Bundle ID comparison is missing the two guards the credentials copy has. Credentials item 4 ends with "Stay silent when apns.bundle_id is null or the project's Bundle ID is unknown", but here the rule is just "compare ... A difference means APNs will reject every push to this app."
_presence emits bundle_id: None whenever apns_bundle_id is absent or empty while apns_env is set, so configured: true with bundle_id: null is reachable, and the curl path prints no line at all for an absent field. An agent following setup alone reads that as a difference and tells the customer push will not deliver, during exactly the iOS onboarding this PR is about. Same for a project with no .xcodeproj yet (Expo before prebuild), where credentials routes the agent to the Xcode-first read and this copy does not.
Could the two guards be copied over, or this sentence reduced to a pointer at item 4?
| The single worst onboarding failure is installing the SDK before push credentials exist: the app builds, the device registers, and it shows up **unsubscribed** because OneSignal has nothing to hand APNs/FCM. Close credentials FIRST. Never skip this step silently. | ||
|
|
||
| 1. **Check what's configured.** With an app-scoped key available (`$ONESIGNAL_SETUP_TOKEN` / `$ONESIGNAL_REST_API_KEY`), run `python3 <plugin>/scripts/onesignal_api.py app <APP_ID>` — the view-app read, `GET /api/v1/apps/{APP_ID}` (app auth works) — and read the verdict for the platform you're about to install: Android → `platforms.fcm.configured`, iOS → `platforms.apns.configured`, Web → `platforms.web.configured` (or the free `web-probe` below). The script decides from the **non-secret presence fields** only (`apns_env`, `fcm_sender_id`, `chrome_web_origin`; api-reference "View an app"), and treats `null`, `""`, and an absent key the same. Without Python (Step 0.2), `curl` the same `GET` with whichever key env var is set, and keep only the HTTP status, the app `id`, and the presence fields — the raw body still carries the plaintext credentials until the server removes them, so never print or paste the whole body: `curl -sS -w '\nHTTP %{http_code}\n' -H "Authorization: Key ${ONESIGNAL_REST_API_KEY:-$ONESIGNAL_SETUP_TOKEN}" "https://api.onesignal.com/api/v1/apps/<APP_ID>" | grep -oE '"(id|apns_env|apns_key_id|apns_team_id|fcm_sender_id|chrome_web_origin)":[^,}]*|^HTTP [0-9]+'`. A verdict exists **only** when the `HTTP` line is 2xx **and** the `"id"` line shows this App ID — that pair is your proof the body is the app object. Then read the presence fields and nothing else — never the plaintext credential fields (`apns_p8`, `fcm_v1_service_account_json`, `gcm_key`). Any other output (no `HTTP` line, a non-2xx status, no `"id"` line, a curl error) is **presence unknown, not "missing"**: do not start the credentials skill on it — take the no-key path below instead. In a sandboxed runtime, a script exit code of 3 or a curl error on the first try means the sandbox blocked the request: re-run that one command with escalation first ("Network access" above), and read presence unknown only if the re-run also fails or the user declines. No key available, or presence unknown → ask the user to check the dashboard and tell you. Send the page as a clickable URL built from the App ID: `https://dashboard.onesignal.com/apps/<APP_ID>/settings/push/platforms` (Settings > Push Platforms). | ||
| 1. **Check what's configured.** With an app-scoped key available (`$ONESIGNAL_SETUP_TOKEN` / `$ONESIGNAL_REST_API_KEY`), run `python3 <plugin>/scripts/onesignal_api.py app <APP_ID>` — the view-app read, `GET /api/v1/apps/{APP_ID}` (app auth works) — and read the verdict for the platform you're about to install: Android → `platforms.fcm.configured`, iOS → `platforms.apns.configured`, Web → `platforms.web.configured` (or the free `web-probe` below). The script decides from the **non-secret presence fields** only (`apns_env`, `fcm_sender_id`, `chrome_web_origin`; api-reference "View an app"), and treats `null`, `""`, and an absent key the same. Without Python (Step 0.2), `curl` the same `GET` with whichever key env var is set, and keep only the HTTP status, the app `id`, and the presence fields — the raw body still carries the plaintext credentials until the server removes them, so never print or paste the whole body: `curl -sS -w '\nHTTP %{http_code}\n' -H "Authorization: Key ${ONESIGNAL_REST_API_KEY:-$ONESIGNAL_SETUP_TOKEN}" "https://api.onesignal.com/api/v1/apps/<APP_ID>" | grep -oE '"(id|apns_env|apns_key_id|apns_team_id|apns_bundle_id|fcm_sender_id|chrome_web_origin)":[^,}]*|^HTTP [0-9]+'`. A verdict exists **only** when the `HTTP` line is 2xx **and** the `"id"` line shows this App ID — that pair is your proof the body is the app object. Then read the presence fields and nothing else — never the plaintext credential fields (`apns_p8`, `fcm_v1_service_account_json`, `gcm_key`). **iOS, when APNs is configured:** compare `platforms.apns.bundle_id` (the `apns_bundle_id` field) with the project's Bundle ID. A difference means APNs will reject every push to this app; say so in one sentence with both values and the dashboard URL below, and keep going (credentials skill Step 1, item 4, has the detail). Any other output (no `HTTP` line, a non-2xx status, no `"id"` line, a curl error) is **presence unknown, not "missing"**: do not start the credentials skill on it — take the no-key path below instead. In a sandboxed runtime, a script exit code of 3 or a curl error on the first try means the sandbox blocked the request: re-run that one command with escalation first ("Network access" above), and read presence unknown only if the re-run also fails or the user declines. No key available, or presence unknown → ask the user to check the dashboard and tell you. Send the page as a clickable URL built from the App ID: `https://dashboard.onesignal.com/apps/<APP_ID>/settings/push/platforms` (Settings > Push Platforms). |
There was a problem hiding this comment.
Separate point on the tail of this same item: it still carries the pre-PR handling for the case item 7 was just rewritten for, "No key available, or presence unknown → ask the user to check the dashboard and tell you." Credentials now sends the URL, asks a §14 structured question, and maps three answers onto configured / not configured / unknown.
This is the same MCP-only-session failure mode, and setup Step 3 is the earlier of the two entry points, so I would expect most sessions to hit this copy rather than the credentials one. Deliberate scoping for this PR, or should setup point at credentials item 7 here?
There was a problem hiding this comment.
Fixed in 58adae7. The no-key and presence-unknown tail now uses the structured question in credentials Step 1, item 7, and applies its rules. "I am not sure" stays unknown, never "missing".
| 5. **Platform not configured →** record that fact, then continue into the matching flow. This record matters later: the 409 disambiguation in the [validation loop](#credential-validation-loop) depends on the platform's state before your first upload attempt, and this check is that state. | ||
| 6. **Wrong or unknown App ID** (`no_such_app` from the web probe, `not_found` from the app read) **→ STOP.** Do not start a portal walkthrough and do not route toward an upload — the endpoint is write-once, and a credential aimed at a mistyped App ID lands on the wrong app. Re-ask the user for the App ID (it is public — grep the init code for it) and re-run this step. A probe `status: unknown` is not a decision either: re-run the probe or fall back to the raw `GET` before you continue. | ||
| 7. **Check cannot run** (no key available, the `GET` itself fails, or the script returns `status: error`) **→** say so and continue into the flow anyway, but record the Step 1 state as **unknown** — never as "not configured". The write-once endpoint still guards the case: an upload against an already-configured platform returns a 409, and the validation loop maps it. With an unknown before-state, that 409 is indeterminate (case (b) in the [validation loop](#credential-validation-loop)), so a recovery re-call cannot claim success from it. | ||
| 7. **Check cannot run** (no key available, the `GET` itself fails, or the script returns `status: error`) **→** say so, in one sentence that names the cause, and ask the user to read the dashboard for you before any portal walkthrough. A session with only the OneSignal MCP connected is the common case: the MCP signs in with OAuth and exposes no view-app read, so this check needs an app-scoped key the session does not have. Send `https://dashboard.onesignal.com/apps/<APP_ID>/settings/push/platforms` and ask one structured question (safety contract §14): "Does the page show Apple iOS (APNs) as configured?" (or Google Android (FCM) for Android), with "Yes, a key is already uploaded", "No, nothing is configured", and "I am not sure" as the choices. Record "Yes" as item 4 and skip the portal walkthrough (on iOS, the dashboard shows the auth type and Bundle ID; ask the user to compare the Bundle ID with the project's if they can), "No" as item 5, and "I am not sure" as **unknown** — never as "not configured". On unknown, continue into the flow anyway. The write-once endpoint still guards the case: an upload against an already-configured platform returns a 409, and the validation loop maps it. With an unknown before-state, that 409 is indeterminate (case (b) in the [validation loop](#credential-validation-loop)), so a recovery re-call cannot claim success from it. |
There was a problem hiding this comment.
Before this change every item 7 outcome recorded credentials.detected fail unknown probe_unavailable, which was an honest "we did not observe this." Now "Yes" routes to item 4 and "No" to item 5, so a user's reading of the dashboard produces the same ok and fail credentials_missing rows a successful view-app read produces, and only "I am not sure" keeps probe_unavailable.
The user-flow half of this reads like a clear improvement. The cost is that credentials.detected ok stops meaning "OneSignal confirmed the credential is stored" and starts meaning "either that, or someone said yes." Since the MCP-only session is described here as the common case, that could be a meaningful share of the rows.
Worth a distinct reason slug on the self-reported path, if the telemetry contract's vocabulary has room for one, so the two stay separable?
There was a problem hiding this comment.
In the telemetry contract, only the auth_resolved milestones can put a reason on an ok row, so adding one here needs a contract change first. I'd still make that change in this PR. Otherwise, once this merges, we can't tell a user's "Yes" from a real API check. "No" has the same problem.
| Full portal detail (screenshots-equivalent steps, .p8-vs-.p12 disambiguation, troubleshooting) is in [api-uploaded-credentials.md](api-uploaded-credentials.md#apple-apns-p8). Summary of the hand-off: | ||
|
|
||
| 1. **Human, in the Apple Developer portal:** Certificates, Identifiers & Profiles → **Keys** → blue **+** → select **Apple Push Notifications service (APNs)** with **Sandbox & Production** → name, Continue, Register → **Download the `.p8`** (one-time download — it cannot be re-downloaded). Requires the **paid** Apple Developer account. | ||
| 1. **Human, in the Apple Developer portal:** Certificates, Identifiers & Profiles → **Keys** → blue **+** → select **Apple Push Notifications service (APNs)** with **Sandbox & Production** → name, Continue, Register → **Download the `.p8`** (one-time download — it cannot be re-downloaded). Requires the **paid** Apple Developer account. **Ask before they create a key:** a `.p8` key belongs to the Apple team, not to one app, so one key serves every app and Bundle ID under the team, and Apple allows at most 2 APNs keys per team. If the team already has an APNs key — the Keys section lists it — reuse that key's `.p8` file and Key ID instead of creating another; only the Bundle ID in step 2 is specific to this app. Two exceptions, both visible on the key's detail page: a key that Apple scoped to one environment must cover **Sandbox & Production**, and a **Topic Specific** key must list this app's Bundle ID. A `.p12` certificate is the other credential type; this endpoint does not accept it (api-uploaded-credentials.md). |
There was a problem hiding this comment.
Two sentences here sit a little oddly together: "Apple allows at most 2 APNs keys per team", then "a Topic Specific key must list this app's Bundle ID."
My understanding is that the 2-key cap applies to Team Scoped (All Topics) keys and that Topic Specific keys are counted separately, but I have not confirmed that against Apple's current documentation. If that is right, the cap sentence reads stricter than it is, and an agent could tell a user they cannot create a key when they can. Worth a check, since this is the sentence that decides whether the user creates a key at all.
There was a problem hiding this comment.
You were right. Apple's documentation gives the limit of 2 for Team Scoped keys in each environment. Topic Specific keys have a separate, much higher limit, and each one serves only the Bundle IDs it lists. Fixed in 58adae7.
|
Correction to my review above, plus one thing that only showed up reviewing this and #83 as the combined stack. The version bump. I wrote "no version bump is needed,
Nothing has. This PR and #83 both change skills, scripts, references, and templates with all 3 fields still at The Bundle ID read, once #83 lands. The two instructions this PR adds to read "the project's Bundle ID" (Step 1 item 4 here, and setup Step 3) are unambiguous against today's The fix most likely belongs in these files, so flagging it here, but the full reasoning and the reachability analysis are on #83 so the discussion stays in one place. |
The credentials and setup skills now compare the stored APNs Bundle ID with the main app target only, never an extension target. The setup copy stays silent on a null or unknown value, and its dashboard fallback uses the credentials question. The APNs key text now gives the Team Scoped limit per environment and the Topic Specific case.
The Create Release PR workflow writes the version into the 3 places, so feature and fix change sets do not bump it.
|
Replies to the review body and the correction comment:
|
|
Did you get to run the eval scenarios for this one? The testing table only shows the script test. The two I'd most want are an MCP-only session and an iOS project with an NSE target. How did they go? |
One-Line Summary
The credentials check now compares the stored APNs Bundle ID with the project's Bundle ID, and the no-key case asks the user what the dashboard shows instead of a portal walkthrough.
Ticket: SDK-5562
Details
The view-app read already returns
apns_bundle_id, the topic OneSignal sends to APNs with every push;onesignal_api.py appnow prints it asplatforms.apns.bundle_id, and the credentials skill (Step 1, item 4) and the setup skill (Step 3) compare it with the project's Bundle ID when APNs is configured. When the presence check cannot run (an MCP-only session has no view-app read), the credentials skill sends the Push Platforms URL and asks one structured question about what the page shows. The.p8flow now says that a key is team-level, that Apple allows 2 APNs keys per team, and which key scopes do not cover this app.Notes
.p8-first, and.p12is dashboard-only by design. The tester's 2 problems were the missing view-app read in the hosted MCP (list_appsreturns app-table fields only; fixed server-side or not at all) and a Bundle ID that did not match the stored credential, which the presence check did not compare.view_apptool inmcp-httpwould make Step 1 work without a REST key.evals/onesignal-api-test.py) goes to the evals repository in its own PR.Testing
onesignal-api-test.py(local, 52 passed, 2 newbundle_idasserts)credentials_p12Affected areas
skills/setup/SKILL.mdskills/setup/assets/skills/credentials/SKILL.mdskills/verify/SKILL.mdscripts/references/