From 04964ad17a09a89e2a4fda4b0eef364fab48c5b7 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 07:08:42 +0000 Subject: [PATCH 1/2] fix(data-objectstack): parse a dropped-fields entry's `fields` elements and `object` instead of asserting them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The write-warning boundary's structural gate read `Array.isArray(fields) && fields.length > 0` and asserted the entry into `Omit`, which claims a `string[]` of field names and a REQUIRED `object: string`. It read neither. `fields: [42]` reached every subscriber typed as a field name, and an entry with no `object` arrived claiming a string that was not there. Same discipline the sibling `reason` fix applied: parse rather than assert. - `WireDroppedFieldsEntry` now declares exactly what the gate establishes: `{ object?: unknown; fields: unknown[]; reason?: unknown }`. - The gate is named (`isWireDroppedFieldsEntry`), shared by the single-record and batch paths, and requires at least one string element — an entry that names no field has nothing truthful to say. - `asDroppedFieldsNotice` narrows `fields` to its string elements and takes `object` from the wire when it is a string, else from the object the write targeted. It now BUILDS the notice, so the last cast in this seam is gone. - The batch path computes the object name once, for both the notice and the event's `resource`; those were two separate expressions that could disagree. No published type changes: `DroppedFieldsNotice`, `WriteWarningEvent` and `UnrecognizedDropReasonEvent` keep their declared shapes, and a subscriber's `fields: string[]` is now true rather than asserted. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012wwHa4aaFybxXrfmfHioDM --- .../src/droppedFieldsShape.boundary.test.ts | 201 ++++++++++++++++++ packages/data-objectstack/src/index.ts | 147 ++++++++++--- 2 files changed, 313 insertions(+), 35 deletions(-) create mode 100644 packages/data-objectstack/src/droppedFieldsShape.boundary.test.ts diff --git a/packages/data-objectstack/src/droppedFieldsShape.boundary.test.ts b/packages/data-objectstack/src/droppedFieldsShape.boundary.test.ts new file mode 100644 index 0000000000..b80d14e650 --- /dev/null +++ b/packages/data-objectstack/src/droppedFieldsShape.boundary.test.ts @@ -0,0 +1,201 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * The write-warning boundary must PARSE a wire entry's `fields` elements and its + * `object`, not assert them into the spec type on array-shape alone + * (objectui#6889). + * + * Sibling to `droppedFieldsReason.boundary.test.ts`, which pinned the same + * discipline for `reason` (objectui#4934). That fix left two smaller + * over-claims behind, and they are independent of each other: + * + * - **`fields` elements.** The gate read `Array.isArray(fields) && length > 0` + * and asserted the entry into a type whose `fields` is `string[]`, so + * `fields: [42]` reached every subscriber typed as a field name. + * - **`object`.** `Omit` carries the spec's + * REQUIRED `object: string`, and nothing in the gate read `object` at all — + * so an entry that omitted it arrived claiming a string that was not there. + * Required is what MAKES this a gap; an optional `object` would have had + * nothing to over-claim. + * + * What these tests pin is the DISCRIMINATION, not a population: nothing in + * these two repos is known to emit either shape, and whether a real server does + * is unanswerable from here. Each was measured red against the pre-fix boundary + * (see the PR's ablation), and each keeps a live positive control beside it so a + * green run cannot be a silently-empty one. + * + * The repair is deliberately asymmetric with `reason`'s. A reason from the + * future is the EXPECTED direction of version skew, so it earns an explicit + * skew arm carrying the wire value verbatim. `fields` is `z.array(z.string())` + * in the spec and cannot grow a non-string element without a breaking change, + * so a non-string element is off-spec input: refused here, fixed at the + * producer (AGENTS.md #0.1). `object` is neither — the adapter already KNOWS + * which object it wrote to, so a missing one is healed rather than dropped. + */ +import { describe, it, expect, vi } from 'vitest'; +import { ObjectStackAdapter, UNRECOGNIZED_DROP_REASON } from './index'; +import type { WriteWarningEvent } from './index'; + +function makeDS(stub: Record) { + const ds: any = new ObjectStackAdapter({ + baseUrl: 'http://test.local', + fetch: vi.fn(async () => + new Response(JSON.stringify({ success: true, data: { capabilities: {}, routes: {} } }), { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }), + ), + }); + ds.connected = true; + ds.connectionState = 'connected'; + ds.client = { data: stub }; + return ds; +} + +/** Drive one create on `andon` whose response carries `droppedFields`. */ +async function emitOnCreate(droppedFields: unknown[]): Promise { + const create = vi.fn().mockResolvedValue({ record: { id: 'r1' }, droppedFields }); + const ds = makeDS({ create }); + const events: WriteWarningEvent[] = []; + ds.onWriteWarning((e: WriteWarningEvent) => events.push(e)); + await ds.create('andon', { title: 'T' }); + return events; +} + +/** Drive one two-op batch whose response carries a top-level `droppedFields`. */ +async function emitOnBatch(droppedFields: unknown[]): Promise { + const batchTransaction = vi.fn().mockResolvedValue({ + results: [{ id: 'acc1' }, { id: 'inv1' }], + droppedFields, + }); + const ds = makeDS({ batchTransaction }); + ds.atomicBatchCapability = true; + const events: WriteWarningEvent[] = []; + ds.onWriteWarning((e: WriteWarningEvent) => events.push(e)); + await ds.batchTransaction([ + { object: 'account', action: 'create', data: { name: 'Acme' } }, + { object: 'invoice', action: 'update', id: 'inv1', data: { status: 'paid' } }, + ]); + return events; +} + +describe('dropped-fields `fields` elements are parsed at the boundary (#6889)', () => { + it('refuses the non-string elements and KEEPS the string ones', async () => { + const events = await emitOnCreate([ + { object: 'andon', fields: ['type', 42, { name: 'salary' }, null, true], reason: 'readonly' }, + ]); + + expect(events).toHaveLength(1); + const [notice] = events[0].droppedFields; + // The lie this card exists to delete: a subscriber's `string[]` must not be + // handed anything that is not a string. + expect(notice.fields).toEqual(['type']); + for (const f of notice.fields) expect(typeof f).toBe('string'); + // The entry itself survives — refusing an element must not silence the + // warning about the field that WAS named (objectui#3484). + expect(notice.reason).toBe('readonly'); + }); + + it('drops an entry that names no field at all, and keeps one that does', async () => { + // Nothing in `[42, {}]` is a field name, so there is nothing truthful to + // tell the user — same disposition the boundary already gave `fields: []`. + expect(await emitOnCreate([{ object: 'andon', fields: [42, {}], reason: 'readonly' }])).toEqual( + [], + ); + + // CONTROL on the same instrument: one string element is enough to emit, so + // the zero above is a reading and not a broken harness. + const control = await emitOnCreate([ + { object: 'andon', fields: ['type'], reason: 'readonly' }, + ]); + expect(control).toHaveLength(1); + expect(control[0].droppedFields[0].fields).toEqual(['type']); + }); + + it('never silences a sibling entry that is well-formed', async () => { + const events = await emitOnCreate([ + { object: 'andon', fields: [42], reason: 'readonly' }, + { object: 'andon', fields: ['source_method'], reason: 'readonly_when' }, + ]); + + expect(events).toHaveLength(1); + expect(events[0].droppedFields).toHaveLength(1); + expect(events[0].droppedFields[0]).toEqual({ + object: 'andon', + fields: ['source_method'], + reason: 'readonly_when', + }); + }); + + it('applies the same parse on the batch path', async () => { + const events = await emitOnBatch([ + { object: 'invoice', fields: ['tax_rate', 42], reason: 'readonly_when', index: 1 }, + ]); + + expect(events).toHaveLength(1); + expect(events[0].droppedFields[0].fields).toEqual(['tax_rate']); + + // CONTROL: an all-non-string batch entry emits nothing, on the same driver. + expect( + await emitOnBatch([{ object: 'invoice', fields: [42], reason: 'readonly_when', index: 1 }]), + ).toEqual([]); + }); + + it('composes with the `reason` skew arm rather than replacing it (#4934)', async () => { + const events = await emitOnCreate([ + { object: 'andon', fields: ['type', 42], reason: 'some_future_reason' }, + ]); + + expect(events[0].droppedFields[0]).toEqual({ + object: 'andon', + fields: ['type'], + reason: UNRECOGNIZED_DROP_REASON, + unrecognizedReason: 'some_future_reason', + }); + }); +}); + +describe('dropped-fields `object` is parsed at the boundary (#6889)', () => { + it('heals a missing `object` from the resource the write targeted', async () => { + const events = await emitOnCreate([{ fields: ['type'], reason: 'readonly' }]); + + expect(events).toHaveLength(1); + const [notice] = events[0].droppedFields; + // Declared `object: string` on the spec arm — so it must BE a string, and + // the only truthful one available is the object this create wrote to. + expect(typeof notice.object).toBe('string'); + expect(notice.object).toBe('andon'); + }); + + it('heals a non-string `object` the same way', async () => { + const events = await emitOnCreate([{ object: 42, fields: ['type'], reason: 'readonly' }]); + + expect(events[0].droppedFields[0].object).toBe('andon'); + }); + + it('CONTROL — a string `object` on the wire is passed through, not overwritten', async () => { + // The write targets `andon`; the wire says the strip is about `andon_line`. + // Healing must never overrule what the server actually said. + const events = await emitOnCreate([ + { object: 'andon_line', fields: ['type'], reason: 'readonly' }, + ]); + + expect(events[0].droppedFields[0].object).toBe('andon_line'); + }); + + it('heals a missing `object` from the originating op on the batch path', async () => { + const events = await emitOnBatch([{ fields: ['tax_rate'], reason: 'readonly_when', index: 1 }]); + + expect(events).toHaveLength(1); + // The notice and the event describing it must name the SAME object — they + // used to be computed by two separate expressions. + expect(events[0].droppedFields[0].object).toBe('invoice'); + expect(events[0].resource).toBe('invoice'); + }); +}); diff --git a/packages/data-objectstack/src/index.ts b/packages/data-objectstack/src/index.ts index c485403988..1b93160c80 100644 --- a/packages/data-objectstack/src/index.ts +++ b/packages/data-objectstack/src/index.ts @@ -1513,11 +1513,26 @@ export type DroppedFieldsNotice = DroppedFieldsEvent | UnrecognizedDropReasonEve /** * A `droppedFields` entry as it comes OFF THE WIRE: everything a structural - * check can honestly claim about it, and no more. `reason` is `unknown` because - * nothing has parsed it yet — writing `DroppedFieldsEvent` here is the exact - * assertion objectui#4934 exists to delete. + * check can honestly claim about it, and no more. Every field is `unknown` + * until something parses it — writing `DroppedFieldsEvent` for any of them here + * is the exact assertion objectui#4934 exists to delete. + * + * It used to be `Omit & { reason?: unknown }`, + * which was honest about `reason` and dishonest about the other two + * (objectui#6889). `Omit` carried the spec's `fields: string[]` and its + * REQUIRED `object: string` through untouched, while the structural gate below + * read neither: `fields: [42]` and an entry with no `object` at all both passed + * and reached subscribers typed as if they had been checked. Required is what + * makes `object` a gap here, not what closes it — an optional `object` would + * have had nothing to over-claim. + * + * So the declaration now states exactly what {@link isWireDroppedFieldsEntry} + * establishes and nothing else, and {@link asDroppedFieldsNotice} PARSES the + * rest. The published surface — {@link DroppedFieldsNotice}, + * {@link WriteWarningEvent} — is unchanged: a subscriber's `fields: string[]` + * stays `string[]` and is now TRUE rather than asserted. */ -type WireDroppedFieldsEntry = Omit & { reason?: unknown }; +type WireDroppedFieldsEntry = { object?: unknown; fields: unknown[]; reason?: unknown }; /** Whether the wire's `reason` is an arm the installed spec pin declares. */ function isRecognizedDropReason(reason: unknown): reason is DroppedFieldsEvent['reason'] { @@ -1525,19 +1540,73 @@ function isRecognizedDropReason(reason: unknown): reason is DroppedFieldsEvent[' } /** - * Classify ONE wire entry by parsing its `reason` against the spec enum - * (objectui#4934). + * The structural gate: is this wire value an entry that names at least one + * field? + * + * `fields` must be an array (so `unknown[]` is established) carrying at least + * one string (so the parsed notice below names something). An array holding no + * string at all — `fields: [42]`, `fields: []` — reports no field name, and an + * entry that names no field has nothing truthful to tell the user; the + * pre-objectui#6889 gate already dropped the empty case for exactly that + * reason, and this is the same rule one level deeper. * - * A recognized entry is passed through by reference — unchanged, extra - * server-sent keys and all — so this is a classification, not a rewrite; only - * the skew case builds a new object. The cast on that path is the one kind this - * seam may still make: `reason` has just been PARSED, so the claim is proven - * rather than assumed. + * `object` and `reason` are deliberately NOT gated on. Nothing is dropped for + * them: both are parsed afterwards, so a missing `object` or an unnameable + * `reason` still reaches the user as the warning it is (objectui#3484's + * silence is the worse failure). */ -function asDroppedFieldsNotice(entry: WireDroppedFieldsEntry): DroppedFieldsNotice { - if (isRecognizedDropReason(entry.reason)) return entry as DroppedFieldsEvent; +function isWireDroppedFieldsEntry(e: unknown): e is WireDroppedFieldsEntry { + return ( + !!e && + typeof e === 'object' && + Array.isArray((e as { fields?: unknown }).fields) && + ((e as { fields: unknown[] }).fields).some((f) => typeof f === 'string') + ); +} + +/** + * Parse ONE wire entry into a notice (objectui#4934, objectui#6889). + * + * Three parses, one per field the gate above does not establish, and the + * result is built rather than asserted — so the last cast in this seam is gone: + * + * - **`reason`** — checked against the spec enum; anything the installed pin + * cannot name goes to the explicit skew arm with the wire value preserved + * verbatim in `unrecognizedReason` (objectui#4934). + * - **`fields`** — narrowed to its string elements. Deliberately NOT given a + * skew arm like `reason`: a reason from the future is the EXPECTED direction + * of version skew, whereas `fields` is `z.array(z.string())` in the spec and + * cannot grow a non-string element without a breaking change. A non-string + * element is off-spec input, so the contract-first answer (AGENTS.md #0.1) is + * to refuse it here and fix the producer — not to invent a rendering for it. + * Measured on the one reader (`app-shell`'s `writeWarningToast`): today such + * an element degrades to a wrong label — `42`, `[object Object]`, `true`, or + * an empty entry for `null` — never a throw, which is why this is repaired as + * an honesty defect rather than a crash. + * - **`object`** — taken from the wire when it is a string, otherwise from + * `fallbackObject`, which is the object the adapter just wrote to. That is + * not a lenient alias: the caller KNOWS the resource, and the batch path has + * always healed the same hole this way for the event's `resource`. Narrowing + * the gate on `object` instead would drop the entry — and no reader depends + * on it (`writeWarningToast` names fields off `WriteWarningEvent.resource`), + * so dropping would trade a silent user-facing warning for nothing. + * + * Extra server-sent keys still ride along: the spread preserves them, and only + * the three parsed keys are rewritten. + */ +function asDroppedFieldsNotice( + entry: WireDroppedFieldsEntry, + fallbackObject: string, +): DroppedFieldsNotice { + const fields = entry.fields.filter((f): f is string => typeof f === 'string'); + const object = typeof entry.object === 'string' ? entry.object : fallbackObject; + if (isRecognizedDropReason(entry.reason)) { + return { ...entry, object, fields, reason: entry.reason }; + } return { ...entry, + object, + fields, reason: UNRECOGNIZED_DROP_REASON, unrecognizedReason: entry.reason, }; @@ -2549,10 +2618,11 @@ export class ObjectStackAdapter implements DataSource { * whose response type predates `droppedFields`: the field is read structurally * and validated, so an older client (or a backend that never drops) is a no-op. * - * SHAPE decides whether an entry is an event at all (it must carry a non-empty - * `fields`); `reason` is then PARSED against the spec enum and an unrecognized - * one routed to the skew arm — never asserted into the union, and never - * dropped (objectui#4934). + * SHAPE decides whether an entry is an event at all (it must name at least one + * field); `object`, `fields` and `reason` are then PARSED — an unrecognized + * reason routed to the skew arm, a non-string field element refused, a missing + * `object` healed from `resource` — never asserted into the union, and the + * entry itself never dropped for them (objectui#4934, objectui#6889). */ private notifyDroppedFields( operation: 'create' | 'update', @@ -2564,14 +2634,8 @@ export class ObjectStackAdapter implements DataSource { const dropped = (result as { droppedFields?: unknown } | null | undefined)?.droppedFields; if (!Array.isArray(dropped) || dropped.length === 0) return; const valid = dropped - .filter( - (e): e is WireDroppedFieldsEntry => - !!e && - typeof e === 'object' && - Array.isArray((e as WireDroppedFieldsEntry).fields) && - (e as WireDroppedFieldsEntry).fields.length > 0, - ) - .map(asDroppedFieldsNotice); + .filter(isWireDroppedFieldsEntry) + .map((e) => asDroppedFieldsNotice(e, resource)); // A strip that changed nothing is not news — see withoutNoOpDrops (#3484). const stored = (result as { record?: Record } | null | undefined)?.record; const droppedFields = withoutNoOpDrops(valid, sent, stored); @@ -2599,12 +2663,24 @@ export class ObjectStackAdapter implements DataSource { if (!Array.isArray(dropped) || dropped.length === 0) return; const results = (payload as { results?: unknown[] } | null | undefined)?.results; for (const entry of dropped) { - if (!entry || typeof entry !== 'object') continue; - // The cast claims only the structure this loop checks; `reason` stays - // unparsed until `asDroppedFieldsNotice` below (objectui#4934). + // Same gate as the single-record path, so the two agree on what an entry + // even is. The remaining cast adds only `index`, which this loop reads + // and the gate has no opinion about (objectui#4934, objectui#6889). + if (!isWireDroppedFieldsEntry(entry)) continue; const e = entry as WireDroppedFieldsEntry & { index?: number }; - if (!Array.isArray(e.fields) || e.fields.length === 0) continue; const op = typeof e.index === 'number' ? operations[e.index] : undefined; + // Which object this strip is about. The wire's own `object` wins; the + // originating op is the fallback when the wire omitted it or sent a + // non-string. ONE spelling, used for both the notice and the event's + // `resource` — they used to be computed separately, so a wire entry with + // a non-string `object` could put one value on the notice and another on + // the event describing it. + const object = + typeof e.object === 'string' + ? e.object + : typeof op?.object === 'string' + ? op.object + : ''; // Same no-op suppression as the single-record path (#3484). The echoed // row for the originating op is the "stored" side; when the batch echoed // nothing usable, `withoutNoOpDrops` keeps every field. @@ -2612,12 +2688,13 @@ export class ObjectStackAdapter implements DataSource { typeof e.index === 'number' && Array.isArray(results) ? (results[e.index] as Record | undefined) : undefined; - // `reason` is parsed against the spec enum here too — the batch path used - // to re-assert the wire value into the union via the cast above - // (objectui#4934). `index` is deliberately not carried onto the notice: - // it addresses an operation in THIS response, not the strip. + // `object`, `fields` and `reason` are parsed here too — the batch path + // used to re-assert all three into the union via the cast above + // (objectui#4934, objectui#6889). `index` is deliberately not carried onto + // the notice: it addresses an operation in THIS response, not the strip, + // which is why the entry is rebuilt rather than spread. const [live] = withoutNoOpDrops( - [asDroppedFieldsNotice({ object: e.object, fields: e.fields, reason: e.reason })], + [asDroppedFieldsNotice({ fields: e.fields, reason: e.reason }, object)], op?.data as Record | undefined, stored, ); @@ -2627,7 +2704,7 @@ export class ObjectStackAdapter implements DataSource { const operation: 'create' | 'update' = (op?.action ?? 'create') === 'create' ? 'create' : 'update'; this.emitWriteWarning({ operation, - resource: e.object ?? op?.object ?? '', + resource: object, ...(op?.id !== undefined && op?.id !== null ? { id: op.id } : {}), droppedFields: [live], }); From e268018b47f93bb9f74cc2becf8b6375ce1e617c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 07:13:58 +0000 Subject: [PATCH 2/2] chore(changeset): declare the dropped-fields boundary parse Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_012wwHa4aaFybxXrfmfHioDM --- .changeset/wire-dropped-fields-shape-6889.md | 21 ++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 .changeset/wire-dropped-fields-shape-6889.md diff --git a/.changeset/wire-dropped-fields-shape-6889.md b/.changeset/wire-dropped-fields-shape-6889.md new file mode 100644 index 0000000000..51a6de28fd --- /dev/null +++ b/.changeset/wire-dropped-fields-shape-6889.md @@ -0,0 +1,21 @@ +--- +'@object-ui/data-objectstack': patch +--- + +Parse a `droppedFields` wire entry's `fields` elements and its `object` at the +write-warning boundary instead of asserting them. + +The structural gate checked `Array.isArray(fields) && fields.length > 0` and then +asserted the entry into a type declaring `fields: string[]` and a required +`object: string` — reading neither. A response carrying `fields: [42]` reached +`onWriteWarning` subscribers typed as a field name (the shell rendered it as the +label `42`), and an entry that omitted `object` arrived claiming a string that was +not there. + +Now the wire type declares only what the gate establishes, and the notice is +parsed: non-string `fields` elements are refused, an entry naming no field at all +is dropped as `fields: []` already was, and a missing or non-string `object` is +healed from the object the write targeted. Warnings are never silenced for a +field the server really did name. No published type changes — `WriteWarningEvent` +and `DroppedFieldsNotice` keep their shapes, and a subscriber's `fields: string[]` +is now true rather than asserted.