amadan
reconcile never creates a subscription; write down the iOS smoke test
Codex's whole-branch review. reconcile subscribed whenever permission was granted and nothing existed, which is exactly the state after disable — so a person's opt-out was silently undone on their next visit. It now repairs only a subscription that exists; creating is enable's job, from a click. The node suite runs enable → disable → reconcile and the browser drive does the same in Chromium. The spec's manual iOS smoke test now exists as docs/ios-smoke.md: the procedure, expected outcomes per step, and a table for the result. It has not been run; the table says so. Two v0.2 items taken early because they were cheap: Retry-After is clamped at 24 hours (an unbounded delta-seconds overflowed time.Duration into a negative value), and the sender tests now push a payload at the bound through encryption and transport, prove a failed selection is a batch error rather than zero devices, and cover 503 with an HTTP-date Retry-After and 404 pruning.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
7 files changed,
+195
−18
SKILL.md+6 −2browser_test.go+9 −1docs/ios-smoke.md+58 −0js/push.mjs+11 −10js/push.test.mjs+22 −0send.go+17 −5send_test.go+72 −0
diff --git a/SKILL.md b/SKILL.md| index d8ede7f..55d66a3 100644 |
| --- a/SKILL.md |
| +++ b/SKILL.md |
| @@ -67,8 +67,12 @@ await reconcile({ registration, publicKey, save }).catch(console.warn); // every |
| button.onclick = () => enable({ registration, publicKey, save }); // from the click; prompts |
| ``` |
| -`ready` matters: `subscribe()` on a registration whose worker is still |
| -installing rejects with InvalidStateError. `enable` must be called |
| +`reconcile` only repairs a subscription that exists; it never creates |
| +one, because permission stays granted after `disable` and a reconcile |
| +that subscribed whenever it could would undo the person's opt-out on |
| +their next visit. `ready` matters: `subscribe()` on a registration |
| +whose worker is still installing rejects with InvalidStateError. |
| +`enable` must be called |
| synchronously from the click handler — it prompts before its first |
| await, and a prompt after an await is denied. It resolves null when |
| denied. `disable({registration, remove})` removes the server row |
diff --git a/browser_test.go b/browser_test.go| index eef30e8..2102513 100644 |
| --- a/browser_test.go |
| +++ b/browser_test.go |
| @@ -177,5 +177,13 @@ func TestBrowserEnrolmentRoundTripsThroughTheHandlers(t *testing.T) { |
| if rows, _ := f.svc.List(ctx, "drive"); len(rows) != 0 { |
| t.Fatalf("after disable: %+v", rows) |
| } |
| - rig.Screen("#ready", "after enable, reconcile, disable") |
| + // Permission is still granted; the next load's reconcile must not |
| + // undo the opt-out. |
| + if got := evalString(rig, "driver.reconcile()"); got != "" { |
| + t.Fatalf("reconcile after disable re-enrolled: %q", got) |
| + } |
| + if rows, _ := f.svc.List(ctx, "drive"); len(rows) != 0 { |
| + t.Fatalf("reconcile after disable stored: %+v", rows) |
| + } |
| + rig.Screen("#ready", "after enable, reconcile, disable, reconcile") |
| } |
diff --git a/docs/ios-smoke.md b/docs/ios-smoke.md| new file mode 100644 |
| index 0000000..ca59a7b |
| --- /dev/null |
| +++ b/docs/ios-smoke.md |
| @@ -0,0 +1,58 @@ |
| +# iOS smoke test |
| + |
| +The one check no automated test in this repository can make. The |
| +browser drive proves the module, the worker import and the handlers |
| +agree in Chromium with a subscription double; Chromium's own push |
| +subscription cannot be pointed at a test service, and iOS cannot be |
| +driven at all. So this is a person, a phone, and ten minutes, run |
| +before a release that touches the JS halves or the handlers. |
| + |
| +**Last run:** not yet. Record the date, iOS version and outcome at |
| +the bottom when it is. |
| + |
| +## Setup |
| + |
| +1. Deploy the example (or any app wired per `SKILL.md`) at an https |
| + origin the phone can reach. `http://localhost` is a secure context |
| + on the machine itself, not across a network; use a real origin or |
| + a tunnel that terminates TLS. |
| +2. Provision the key once and keep it across restarts: |
| + `EXAMPLE_VAPID_PRIVATE_KEY="$(cat .vapid-key)"`. |
| +3. iPhone or iPad on iOS/iPadOS 16.4 or later, in Safari. |
| + |
| +## Steps and expected outcomes |
| + |
| +1. **Open the origin in Safari.** The page loads; the coaching |
| + paragraph about the Home Screen is visible; the buttons are |
| + disabled (Safari-in-browser reports `push: false` on iOS, so the |
| + log says the browser cannot do push). |
| +2. **Share → Add to Home Screen. Open the installed copy.** The |
| + coaching paragraph is hidden (`standalone` true); after the worker |
| + activates, the buttons are enabled; the log shows `subscribed: |
| + false`. |
| +3. **Tap Enable.** iOS shows its permission prompt (it must come from |
| + the tap — if no prompt appears, the gesture was lost). Allow. The |
| + log shows `enabled: https://web.push.apple.com/…`. On the server, |
| + one row for `dev`. |
| +4. **Tap Notify me.** A notification appears in the system tray with |
| + the title "Hello from aviso" while the app is in the foreground and |
| + again after backgrounding the app and tapping Notify from another |
| + device or a curl to `/notify`. |
| +5. **Tap the notification.** The installed app comes to the front at |
| + `/` (the payload's `url`). |
| +6. **Tap Disable.** The log shows `disabled`; the server has zero |
| + rows. Close the app fully and reopen it: the log shows `subscribed: |
| + false` and the server still has zero rows — reconcile must not |
| + re-enrol after an opt-out. |
| +7. **Tap Enable again, then rotate the key on the server** (new |
| + `EXAMPLE_VAPID_PRIVATE_KEY`, restart). Reopen the app: reconcile |
| + re-subscribes under the new key without a prompt, and Notify |
| + delivers again. |
| +8. **Remove the app from the Home Screen.** The next send reports 410 |
| + from Apple's push service and the row is pruned. |
| + |
| +## Recording a run |
| + |
| +| Date | iOS | Device | Steps passed | Notes | |
| +|------|-----|--------|--------------|-------| |
| +| | | | | | |
diff --git a/js/push.mjs b/js/push.mjs| index 7bd3f26..5e0d62e 100644 |
| --- a/js/push.mjs |
| +++ b/js/push.mjs |
| @@ -101,22 +101,23 @@ export async function enable({ registration, publicKey, save }, env = globalThis |
| return sub; |
| } |
| -// reconcile repairs on every page load without prompting: with |
| -// permission granted it re-subscribes if the server key changed and |
| -// re-saves so the server's last_confirmed_at moves. Resolves the |
| -// subscription or null. |
| +// reconcile repairs an EXISTING subscription on every page load |
| +// without prompting: re-saves it so the server's last_confirmed_at |
| +// moves, or re-subscribes if the server key changed. It never creates |
| +// one from nothing — permission stays granted after disable(), and a |
| +// reconcile that subscribed whenever it could would silently undo the |
| +// person's opt-out on their next visit. Creating is enable's job. |
| +// Resolves the subscription or null. |
| export async function reconcile({ registration, publicKey, save }, env = globalThis) { |
| if (!env.Notification || env.Notification.permission !== "granted") return null; |
| const existing = await registration.pushManager.getSubscription(); |
| - if (existing && sameKey(existing, publicKey)) { |
| + if (!existing) return null; |
| + if (sameKey(existing, publicKey)) { |
| await persist(save, body(existing, publicKey, "")); |
| return existing; |
| } |
| - let previous = ""; |
| - if (existing) { |
| - previous = existing.endpoint; |
| - await existing.unsubscribe(); |
| - } |
| + const previous = existing.endpoint; |
| + await existing.unsubscribe(); |
| const sub = await registration.pushManager.subscribe({ |
| userVisibleOnly: true, |
| applicationServerKey: toBytes(publicKey), |
diff --git a/js/push.test.mjs b/js/push.test.mjs| index faf8a22..4aceba6 100644 |
| --- a/js/push.test.mjs |
| +++ b/js/push.test.mjs |
| @@ -146,6 +146,28 @@ test("reconcile re-subscribes under a new key and names the old endpoint", async |
| assert.equal(saved[0].subscription.endpoint, "https://push.example/new"); |
| }); |
| +test("reconcile never creates a subscription: enable, disable, reconcile stays disabled", async () => { |
| + const e = env("granted"); |
| + let current = null; |
| + const reg = { |
| + pushManager: { |
| + async getSubscription() { return current; }, |
| + async subscribe(opts) { current = fakeSub("https://push.example/new", new Uint8Array(opts.applicationServerKey)); return current; }, |
| + }, |
| + }; |
| + const { saved, save } = okSave(); |
| + const removed = []; |
| + const remove = async (b) => { removed.push(b.endpoint); return { ok: true }; }; |
| + assert.ok(await enable({ registration: reg, publicKey: KEY, save }, e)); |
| + current.unsubscribe = async () => { current = null; return true; }; |
| + await disable({ registration: reg, remove }); |
| + assert.deepEqual(removed, ["https://push.example/new"]); |
| + // Permission is still "granted"; the next page load must not re-enrol. |
| + assert.equal(await reconcile({ registration: reg, publicKey: KEY, save }, e), null); |
| + assert.equal(current, null); |
| + assert.equal(saved.length, 1, "reconcile saved after disable"); |
| +}); |
| + |
| test("reconcile does nothing without permission", async () => { |
| for (const perm of ["default", "denied"]) { |
| const e = env(perm); |
diff --git a/send.go b/send.go| index 210fcbc..c79baba 100644 |
| --- a/send.go |
| +++ b/send.go |
| @@ -247,15 +247,27 @@ func redact(err error) error { |
| return ErrTransport |
| } |
| +// maxRetryAfter caps what a push service can ask for: a value past |
| +// this is meaningless to a scheduler, and an unbounded one overflows |
| +// time.Duration into a negative number. |
| +const maxRetryAfter = 24 * time.Hour |
| + |
| func parseRetryAfter(v string, now time.Time) time.Duration { |
| if v == "" { |
| return 0 |
| } |
| - if secs, err := strconv.Atoi(v); err == nil && secs >= 0 { |
| - return time.Duration(secs) * time.Second |
| + var d time.Duration |
| + if strings.Trim(v, "0123456789") == "" { // delta-seconds, however many digits |
| + secs, err := strconv.ParseInt(v, 10, 64) |
| + if err != nil || secs > int64(maxRetryAfter/time.Second) { // err here is only ever "out of range" |
| + return maxRetryAfter |
| + } |
| + d = time.Duration(secs) * time.Second |
| + } else if t, err := http.ParseTime(v); err == nil && t.After(now) { |
| + d = t.Sub(now) |
| } |
| - if t, err := http.ParseTime(v); err == nil && t.After(now) { |
| - return t.Sub(now) |
| + if d > maxRetryAfter { |
| + return maxRetryAfter |
| } |
| - return 0 |
| + return d |
| } |
diff --git a/send_test.go b/send_test.go| index 2074ab7..b7e3fbb 100644 |
| --- a/send_test.go |
| +++ b/send_test.go |
| @@ -176,6 +176,78 @@ func TestSendReportsRetryAfter(t *testing.T) { |
| } |
| } |
| +func TestParseRetryAfter(t *testing.T) { |
| + now := time.Date(2026, 9, 7, 12, 0, 0, 0, time.UTC) |
| + for in, want := range map[string]time.Duration{ |
| + "": 0, |
| + "120": 120 * time.Second, |
| + "-5": 0, |
| + "nonsense": 0, |
| + "9223372037": maxRetryAfter, // would overflow time.Duration |
| + "99999999999999999999": maxRetryAfter, |
| + "Mon, 07 Sep 2026 12:05:00 GMT": 5 * time.Minute, |
| + "Mon, 07 Sep 2026 11:00:00 GMT": 0, // in the past |
| + "Wed, 07 Oct 2026 12:00:00 GMT": maxRetryAfter, |
| + } { |
| + if got := parseRetryAfter(in, now); got != want { |
| + t.Errorf("%q: got %v, want %v", in, got, want) |
| + } |
| + } |
| +} |
| + |
| +// A payload at the bound must actually encrypt and travel, not merely |
| +// pass validation with no recipients. |
| +func TestSendDeliversAPayloadAtTheBound(t *testing.T) { |
| + p := newPushRecorder(t) |
| + s := serviceAgainst(t, p) |
| + ctx := context.Background() |
| + _ = s.put(ctx, "alice", sub(p.url("/one")), "") |
| + res, err := s.SendTo(ctx, "alice", make([]byte, maxPayload), Options{}) |
| + if err != nil || len(res) != 1 || res[0].Err != nil || res[0].Status != 201 { |
| + t.Fatalf("res=%+v err=%v", res, err) |
| + } |
| + <-p.hdr |
| +} |
| + |
| +// A failed selection is a batch error, never "zero devices". |
| +func TestSendToReportsASelectionFailure(t *testing.T) { |
| + s := newInternalService(t) |
| + ctx := context.Background() |
| + _ = s.put(ctx, "alice", sub("https://push.example/one"), "") |
| + if _, err := s.cfg.DB.Exec(`DROP TABLE aviso_subscriptions`); err != nil { |
| + t.Fatal(err) |
| + } |
| + res, err := s.SendTo(ctx, "alice", []byte("x"), Options{}) |
| + if err == nil || res != nil { |
| + t.Fatalf("res=%+v err=%v; want nil results and a batch error", res, err) |
| + } |
| +} |
| + |
| +func TestSendReportsServiceUnavailableAndPrunesNotFound(t *testing.T) { |
| + p := newPushRecorder(t) |
| + s := serviceAgainst(t, p) |
| + ctx := context.Background() |
| + _ = s.put(ctx, "alice", sub(p.url("/one")), "") |
| + p.retry = "Mon, 07 Sep 2026 12:05:00 GMT" |
| + p.status.Store(503) |
| + s.now = func() time.Time { return time.Date(2026, 9, 7, 12, 0, 0, 0, time.UTC) } |
| + res, _ := s.SendTo(ctx, "alice", []byte("x"), Options{}) |
| + if res[0].Status != 503 || res[0].RetryAfter != 5*time.Minute || res[0].Err == nil { |
| + t.Fatalf("503: %+v", res[0]) |
| + } |
| + if rows, _ := s.List(ctx, "alice"); len(rows) != 1 { |
| + t.Fatal("503 pruned") |
| + } |
| + p.status.Store(404) |
| + res, _ = s.SendTo(ctx, "alice", []byte("x"), Options{}) |
| + if res[0].Status != 404 || res[0].Err == nil { |
| + t.Fatalf("404: %+v", res[0]) |
| + } |
| + if rows, _ := s.List(ctx, "alice"); len(rows) != 0 { |
| + t.Fatal("404 did not prune") |
| + } |
| +} |
| + |
| func TestSendSkipsRowsUnderAnotherKey(t *testing.T) { |
| p := newPushRecorder(t) |
| s := serviceAgainst(t, p) |