From 52628392bef3bfd2eb83f849508d4f61cf36180d Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Mon, 21 Sep 2026 07:26:01 -0400 Subject: [PATCH] fix(mobile): reject a page storage write for size only, and log the rest (OTA phase C, C7.7 round 1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ruling 33.4. `PageStorageRefusedError` was raised for all three refusals, and two of them have no catcher: a page-closure writer of an unlisted key awaits `setItem` with nothing around it — `notification-delivery-preferences.ts:39` plainly, `preferences.ts` in several places — so a key the page was never allowed to keep became an unhandled rejection in the document. That is a worse failure than the silent drop it replaced, and it is the one the page can least afford, because an uncaught rejection there is a document-level error on a screen that is otherwise working. Scope is now one refusal. `too-large` rejects, because the caller that needs it is written for it: the durable send journal's composer catches it and answers "Message not sent" rather than sending a mutation whose operation id was never written down (ruling 7). `not-allowed` and `not-delivered` resolve and are logged as `[page-bridge] storage-write-dropped`, which is the old behaviour plus the line a device log needs — a preference that did not stick looks identical to one nobody set. A batch applies every pair it can, logs every drop, and rejects only if one of them was oversize. Red first, measured here: seven cases in `page-async-storage.test.ts` red on the rejection, among them a `notificationDeliveryPreferences` write resolving, another host's pins, another workspace's chat tabs, and a write the shell would not take. The oversize case is unchanged and still asserts `PageStorageRefusedError` with the key and the character bound in its message, so the narrowing is visible as the difference between the two. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb --- .../bridge/page-async-storage.test.ts | 69 +++++++++++-------- .../bridge/page-async-storage.ts | 50 ++++++++++---- 2 files changed, 78 insertions(+), 41 deletions(-) diff --git a/mobile/src/mobile-web-shell/bridge/page-async-storage.test.ts b/mobile/src/mobile-web-shell/bridge/page-async-storage.test.ts index 9c4cc76dbae..256f4039222 100644 --- a/mobile/src/mobile-web-shell/bridge/page-async-storage.test.ts +++ b/mobile/src/mobile-web-shell/bridge/page-async-storage.test.ts @@ -84,33 +84,27 @@ describe('a write the page makes', () => { await expect(pageAsyncStorage.getItem('orca:pins:host-1')).resolves.toBeNull() }) - it('is refused, and not kept, for a key outside the allowlist', async () => { + it('is dropped, and not kept, for a key outside the allowlist', async () => { // Held locally it would answer a later read with a value no other screen in the app can see — - // a pin that looks set and is not, which is the failure the grant exists to avoid. - const refusal = await refusalOf(pageAsyncStorage.setItem('orca:mobileWebShellEnabled', 'true')) - expect(refusal.refusal).toBe('not-allowed') + // a pin that looks set and is not, which is the failure the grant exists to avoid. Dropped + // rather than rejected: ruling 33.4, and the case at the end of this file says why. + await expect( + pageAsyncStorage.setItem('orca:mobileWebShellEnabled', 'true') + ).resolves.toBeUndefined() expect(writes).toEqual([]) await expect(pageAsyncStorage.getItem('orca:mobileWebShellEnabled')).resolves.toBeNull() }) - it('is refused, and not kept, when the shell granted no storage', async () => { - granted = false - const refusal = await refusalOf(pageAsyncStorage.setItem('orca:pins:host-1', '["wt-1"]')) - expect(refusal.refusal).toBe('not-delivered') - await expect(pageAsyncStorage.getItem('orca:pins:host-1')).resolves.toBeNull() - }) - - it('carries each pair of a multi-write separately, and refuses the ones outside the list', async () => { - const refusal = await refusalOf( + it('carries each pair of a multi-write separately, and drops the ones outside the list', async () => { + await expect( pageAsyncStorage.multiSet([ ['orca:pins:host-1', '["wt-1"]'], ['orca:remotePushHostRegistrations', '{}'] ]) - ) - // The pair it could apply is applied before the batch rejects: a caller retrying the whole - // batch after a refusal must not find the good half missing as well. + ).resolves.toBeUndefined() + // The pair it could apply is applied: a batch that dropped the good half as well would lose a + // write nothing was wrong with. expect(writes).toEqual([{ key: 'orca:pins:host-1', value: '["wt-1"]' }]) - expect(refusal.key).toBe('orca:remotePushHostRegistrations') }) it('never empties the app store, which is not this document to empty', async () => { @@ -122,24 +116,24 @@ describe('a write the page makes', () => { }) describe('what the page will not keep', () => { - it("refuses another host's pinned list, so a later read cannot answer with it", async () => { + it("drops another host's pinned list, so a later read cannot answer with it", async () => { publish({ 'orca:pins:host-1': '["mine"]' }) - const refusal = await refusalOf(pageAsyncStorage.setItem('orca:pins:host-2', '["theirs"]')) + await expect( + pageAsyncStorage.setItem('orca:pins:host-2', '["theirs"]') + ).resolves.toBeUndefined() // Nothing posted, and nothing cached: a value held here that the shell will not write is a pin // that looks set to this document and to nothing else in the app. - expect(refusal.refusal).toBe('not-allowed') expect(writes).toEqual([]) expect(await pageAsyncStorage.getItem('orca:pins:host-2')).toBeNull() }) - it("refuses another workspace's chat tabs on the session route it was not opened for", async () => { + it("drops another workspace's chat tabs on the session route it was not opened for", async () => { publish() - const refusal = await refusalOf( + await expect( pageAsyncStorage.setItem('orca:nativeChatTabs:host-1:wt-2', '{}') - ) - expect(refusal.refusal).toBe('not-allowed') + ).resolves.toBeUndefined() expect(writes).toEqual([]) - // And the one it was opened for goes through, so the refusal above is about the workspace. + // And the one it was opened for goes through, so the drop above is about the workspace. await pageAsyncStorage.setItem('orca:nativeChatTabs:host-1:wt-1', '{}') expect(writes).toEqual([{ key: 'orca:nativeChatTabs:host-1:wt-1', value: '{}' }]) }) @@ -167,10 +161,29 @@ describe('what the page will not keep', () => { expect(await pageAsyncStorage.getItem('orca:last-visited-worktree')).toBe(atBound) }) - it('refuses a removal it may not make, rather than resolving over it', async () => { + it('drops a write it may not make rather than rejecting, because nobody catches one', async () => { + // Ruling 33.4. Every page-closure writer of an unlisted key calls `setItem` with no catch — + // `notification-delivery-preferences.ts:39` awaits it inside a function its callers `void` — + // so rejecting here turns a dropped preference into an unhandled rejection in the page. The + // drop is the old behaviour and the right one; only the journal's oversize path rejects, + // because the composer is written to catch that one. publish() - const refusal = await refusalOf(pageAsyncStorage.removeItem('orca:pins:host-2')) - expect(refusal.refusal).toBe('not-allowed') + await expect( + pageAsyncStorage.setItem('orca:notificationDeliveryPreferences', '{}') + ).resolves.toBeUndefined() + expect(writes).toEqual([]) + expect(await pageAsyncStorage.getItem('orca:notificationDeliveryPreferences')).toBeNull() + }) + + it('drops a removal it may not make, for the same reason', async () => { + publish() + await expect(pageAsyncStorage.removeItem('orca:pins:host-2')).resolves.toBeUndefined() expect(writes).toEqual([]) }) + + it('drops a write the shell would not take, rather than rejecting', async () => { + granted = false + await expect(pageAsyncStorage.setItem('orca:pins:host-1', '["wt-1"]')).resolves.toBeUndefined() + await expect(pageAsyncStorage.getItem('orca:pins:host-1')).resolves.toBeNull() + }) }) diff --git a/mobile/src/mobile-web-shell/bridge/page-async-storage.ts b/mobile/src/mobile-web-shell/bridge/page-async-storage.ts index 9649a9f3039..961c8cb67c8 100644 --- a/mobile/src/mobile-web-shell/bridge/page-async-storage.ts +++ b/mobile/src/mobile-web-shell/bridge/page-async-storage.ts @@ -27,12 +27,19 @@ const REFUSAL_SENTENCES: Record = { /** * A write the page could not make, as something a screen can put on itself. * - * The real AsyncStorage rejects when its store refuses — a value over the row limit is a SQLite - * error on Android — so rejecting is the module's own contract rather than a shape invented here, - * and every caller that already catches a save gets the refusal for free. The one that matters is - * the durable send journal: `mobile-structured-agent-session-send.ts` catches it and answers - * "Message not sent" instead of sending a mutation whose operation id was never written down - * (rulings-ota-c7.md ruling 7). + * Raised for one refusal only, `too-large`, and that scope is the whole of ruling 33.4. The real + * AsyncStorage rejects when its store refuses — a value over the row limit is a SQLite error on + * Android — so rejecting is the module's own contract for a value too big, and the caller that + * needs it is written for it: the durable send journal, whose composer catches this and answers + * "Message not sent" rather than sending a mutation whose operation id was never written down + * (ruling 7). + * + * The other two refusals stay silent drops, because nothing catches them. A page-closure writer of + * an unlisted key calls `setItem` and awaits it with no catch — + * `notification-delivery-preferences.ts:39` is the plain case, and `preferences.ts` has several — + * so rejecting there converts a preference the page was never allowed to keep into an unhandled + * rejection in the document. A write nobody may make and a write the shell would not take are both + * the page failing to change anything, which is what it already does; the log is where they go. */ export class PageStorageRefusedError extends Error { readonly refusal: PageStorageRefusal @@ -96,17 +103,34 @@ function accept(key: string, value: string | null): PageStorageRefusal | null { return null } -/** One refusal, as the rejection the caller's own catch is written for. */ +/** + * One refusal: the rejection the composer's catch is written for, or a logged drop. + * + * Logged and not silent, because the drop is the thing a reader of a device log has to be able to + * find — a preference that did not stick looks identical to one nobody set. + */ function settle(key: string, refusal: PageStorageRefusal | null): Promise { - return refusal === null - ? Promise.resolve() - : Promise.reject(new PageStorageRefusedError(key, refusal)) + if (refusal === null) { + return Promise.resolve() + } + if (refusal === 'too-large') { + return Promise.reject(new PageStorageRefusedError(key, refusal)) + } + console.warn('[page-bridge] storage-write-dropped', { key, refusal }) + return Promise.resolve() } -/** The first refusal of a batch, after every pair that could be applied has been. */ +/** The first refusal of a batch, after every pair that could be applied has been. Every dropped + * pair is logged by `settle`; only an oversize one can reject, and it does so after the rest. */ function settleBatch(refusals: { key: string; refusal: PageStorageRefusal }[]): Promise { - const first = refusals[0] - return first === undefined ? Promise.resolve() : settle(first.key, first.refusal) + let rejection: Promise | null = null + for (const entry of refusals) { + const settled = settle(entry.key, entry.refusal) + if (entry.refusal === 'too-large' && rejection === null) { + rejection = settled + } + } + return rejection ?? Promise.resolve() } const pageAsyncStorage = {