fix(app-shell): make MetadataService.saveObject's fields argument required

.changeset/6490-saveobject-fields-required.md

@@ -0,0 +1,42 @@
+---
+'@object-ui/app-shell': minor
+---
+
+**BREAKING (in name only):** `MetadataService.saveObject(obj, existingFields)` now requires
+its second argument (objectui#6490). Calls that omitted it no longer compile.
+
+**Runtime behaviour is unchanged, and that is the whole justification.** A call that omitted
+the field list was already a guaranteed `422` — every time it ran, against every backend.
+`ObjectSchema.fields` is not merely typed, it is REQUIRED: measured against the installed
+`@objectstack/spec` 17.2.0, `ObjectSchema.safeParse({ name: 'account', label: 'Account' })`
+fails with `invalid_type @ fields`, and `metadata-protocol`'s `saveMetaItem` parses the whole
+item against that same schema and throws `422 INVALID_METADATA` **before** it persists. The
+method cannot build a valid document without the argument, so the only calls this break
+breaks are calls that already failed. The signature is now honest about it, and the diagnosis
+moves from a round trip at runtime to the compiler.
+
+Nothing new is exported and nothing new is accepted — this narrows the published surface
+rather than widening it. In-repo production call sites were measured at **zero** (only tests
+called it), so the migration for an external consumer is to pass the field list it was
+already required to send: `saveObject(obj, fields)`.
+
+⛔ Two readings were considered and declined, recorded so neither is taken later as a
+shortcut. **Not a `{}` default** — `{}` parses GREEN and `PUT /api/v1/meta/object/:name` is
+an upsert, so defaulting would delete every field of the object on a save that only meant to
+rename it, trading a loud, harmless 422 for silent data loss; the anti-wipe control from
+objectui#6240 (`omits fields entirely when the caller supplied none — it does NOT write {}`)
+moves with the signature and still guards the path a JavaScript consumer can reach. **Not
+fetch-and-merge** — an object save could GET the current document and preserve its stored
+`fields` the way `saveFields` does, but that builds capability for a path with zero measured
+pull and makes the parameter redundant.
+
+An EMPTY list stays a different statement from a missing one: `[]` means "this object has no
+fields", writes `{}`, and under the upsert performs the wipe the caller asked for — the same
+authoritative reading `saveFields` gives its own empty list. Unchanged, and now pinned,
+because the required parameter is what routes a caller with nothing to hand toward it.
+
+Scored `minor` and not `major` per AGENTS.md §版本号策略 — objectui's major is pinned to the
+`@objectstack` major so that "same major ⇒ compatible" holds across the two repos, and every
+publishable package sits in one `fixed` group, so objectui's own breaking changes ship as
+`minor` with the break spelled out in the body. That is the convention, which is why the
+break is stated in words above.

packages/app-shell/src/services/MetadataService.objectPayloadFieldsMap.test.ts

@@ -217,7 +217,24 @@ describe('objectui#6240 · saveObject PUTs `fields` as a name-keyed map', () =>
     // caller that simply did not pass `existingFields` would delete every field
     // of the object. The body stays refused instead — unchanged from before
     // this card, and deliberately so.
+    //
+    // objectui#6490 made `existingFields` REQUIRED, and this control MOVED with
+    // the signature rather than being deleted as newly-unreachable. Both halves
+    // of it are still live, and they measure different things:
+    //
+    //   1. The `@ts-expect-error` below is the COMPILE-TIME half of #6490. This
+    //      project (`tsconfig.test.json`) compiles this file, and an unused
+    //      `@ts-expect-error` is itself an error — so the line reds if the
+    //      argument ever goes back to optional. That is the only direction the
+    //      narrowing can regress in, and this is what pins it.
+    //   2. The RUNTIME half is unchanged and still reachable. `MetadataService`
+    //      is public API (app-shell exports `useMetadataService`), so a
+    //      JavaScript consumer — or a TypeScript one that casts past the types —
+    //      can still arrive here with no field list. What it must get is the
+    //      loud, harmless 422 below, never the silent wipe.
     const { adapter, puts } = makeCapturingAdapter();
+    // @ts-expect-error objectui#6490 — `existingFields` is required. This call is
+    // exactly what the new signature refuses, and it is compiled to prove it does.
     await new MetadataService(adapter).saveObject(ACCOUNT);
 
     expect(puts).toHaveLength(1);
@@ -226,6 +243,24 @@ describe('objectui#6240 · saveObject PUTs `fields` as a name-keyed map', () =>
     // Positive control: the rest of the object still went out.
     expect(puts[0]).toMatchObject({ name: 'account', label: 'Account', pluralLabel: 'Accounts' });
   });
+
+  it('objectui#6490 · an EMPTY list is a STATEMENT, not a missing argument — it writes `{}`', async () => {
+    // The cell the required parameter makes canonical: with the argument no
+    // longer omittable, `[]` is the only spelling left for "no fields", and it
+    // means what it says. Unchanged behaviour — `[]` is truthy, so it has always
+    // converted to `{}` — pinned now because the signature change is what routes
+    // callers to it. The two readings must stay distinguishable: the control
+    // above is about a MISSING argument and never about an empty one, and
+    // `saveFields` reads its own empty list the same authoritative way.
+    const { adapter, puts } = makeCapturingAdapter();
+    await new MetadataService(adapter).saveObject(ACCOUNT, []);
+
+    expect(puts).toHaveLength(1);
+    expect(puts[0].fields).toEqual({});
+    // And unlike the missing-argument body, this one is one the server ACCEPTS —
+    // which is the whole hazard, and why it may only ever happen on request.
+    expect(issuesOf(ObjectSchema.safeParse(puts[0]))).toEqual([]);
+  });
 });
 
 describe('objectui#6240 · saveFields no longer converts the server’s map INTO an array', () => {

packages/app-shell/src/services/MetadataService.saveAdvisories.test.ts

@@ -34,7 +34,7 @@
 import { describe, it, expect, vi } from 'vitest';
 import { ObjectStackAdapter, type MetadataSaveAdvisoryEvent } from '@object-ui/data-objectstack';
 import type { ObjectDefinition } from '@object-ui/types';
-import { MetadataService } from './MetadataService';
+import { MetadataService, type FieldMetadataPayload } from './MetadataService';
 import { emitSaveAdvisories, type TranslateFn } from '../providers/saveAdvisoryToast';
 
 const PURGE_ADVISORY = {
@@ -100,14 +100,23 @@ function makeWiredAdapter(body: unknown) {
 // (objectui#4040). Declared properly instead of widening the cast.
 const ACCOUNT: ObjectDefinition = { id: 'account', name: 'account', label: 'Account' };
 
+// objectui#6490 made that second parameter REQUIRED. `ObjectSchema.fields` is a
+// required record, so a `saveObject` call that omitted the list built a body the
+// server refused `422 INVALID_METADATA` every time it ran — these suites were
+// asserting the advisory channel on top of a save that could never have
+// succeeded against a real backend. The field list is not this file's subject,
+// so it hands over the smallest well-formed one; nothing else about these
+// assertions changes.
+const ACCOUNT_FIELDS: FieldMetadataPayload[] = [{ name: 'name', type: 'text', label: 'Name' }];
+
 describe('MetadataService saves reach the shell advisory surface (#4237)', () => {
   it('renders the gate findings for a save that succeeded', async () => {
     const { adapter, sink, events } = makeWiredAdapter({
       ...CLEAN_BODY,
       advisories: [PURGE_ADVISORY],
     });
 
-    await new MetadataService(adapter).saveObject(ACCOUNT);
+    await new MetadataService(adapter).saveObject(ACCOUNT, ACCOUNT_FIELDS);
 
     expect(events).toHaveLength(1);
     expect(events[0]).toMatchObject({ type: 'object', name: 'account', mode: 'publish' });
@@ -117,7 +126,7 @@ describe('MetadataService saves reach the shell advisory surface (#4237)', () =>
   it('lands on the WARNING tier and says "Saved" first — the write succeeded', async () => {
     const { adapter, sink } = makeWiredAdapter({ ...CLEAN_BODY, advisories: [PURGE_ADVISORY] });
 
-    await new MetadataService(adapter).saveObject(ACCOUNT);
+    await new MetadataService(adapter).saveObject(ACCOUNT, ACCOUNT_FIELDS);
 
     const [title, opts] = sink.warning.mock.calls[0]!;
     expect(title).toMatch(/^Saved/);
@@ -130,7 +139,7 @@ describe('MetadataService saves reach the shell advisory surface (#4237)', () =>
   it('a clean save renders no new UI', async () => {
     const { adapter, sink, events } = makeWiredAdapter(CLEAN_BODY);
 
-    await new MetadataService(adapter).saveObject(ACCOUNT);
+    await new MetadataService(adapter).saveObject(ACCOUNT, ACCOUNT_FIELDS);
 
     expect(events).toEqual([]);
     expect(sink.warning).not.toHaveBeenCalled();
@@ -152,7 +161,7 @@ describe('MetadataService saves reach the shell advisory surface (#4237)', () =>
     const { adapter } = makeWiredAdapter({ ...CLEAN_BODY, advisories: [PURGE_ADVISORY] });
     const invalidate = vi.spyOn(adapter, 'invalidateCache');
 
-    await expect(new MetadataService(adapter).saveObject(ACCOUNT)).resolves.toBeUndefined();
+    await expect(new MetadataService(adapter).saveObject(ACCOUNT, ACCOUNT_FIELDS)).resolves.toBeUndefined();
 
     // The service's own post-save step still runs.
     expect(invalidate).toHaveBeenCalledWith('object:account');

packages/app-shell/src/services/MetadataService.ts

@@ -171,6 +171,13 @@ function toObjectPayload(obj: ObjectDefinition, fields?: FieldMetadataPayload[])
     // right outcome for a caller that under-specified an upsert, and the same
     // outcome as before this change. `saveFields` is the opposite case and
     // treats its argument as authoritative; see there.
+    //
+    // This parameter stays OPTIONAL after objectui#6490 made `saveObject`'s
+    // public one required, and the optionality is the point rather than a
+    // leftover: the type now says no in-repo caller can reach this branch, and
+    // the branch is what a caller who ignores the types still lands in. It is
+    // the last line of defence against the wipe, so it may not be deleted as
+    // newly-unreachable code.
     fields: fields ? toFieldsMap(fields) : undefined,
   };
 }
@@ -493,8 +500,47 @@ export class MetadataService {
   /**
    * Persist an object definition to the backend.
    * Works for both create and update (the API is an upsert).
+   *
+   * ## Why `existingFields` is REQUIRED (objectui#6490)
+   *
+   * `ObjectSchema.fields` is not merely typed, it is REQUIRED. Measured against
+   * the installed `@objectstack/spec` 17.2.0:
+   *
+   *   ObjectSchema.safeParse({ name: 'account', label: 'Account' })
+   *     => success = false   invalid_type @ fields
+   *
+   * and `metadata-protocol`'s `saveMetaItem` parses the whole item against that
+   * same schema and throws BEFORE it persists. So a call that omitted the field
+   * list built a body the server was guaranteed to refuse with
+   * `422 INVALID_METADATA` — this method cannot write a valid document without
+   * it. Requiring the argument moves that guaranteed RUNTIME failure to compile
+   * time; runtime accept/reject is unchanged, because the only calls it breaks
+   * are calls that already failed, every time they ran.
+   *
+   * An EMPTY list is a different statement from a missing one, and it is
+   * accepted: `[]` says "this object has no fields", writes `{}`, and under the
+   * upsert that is a field wipe the caller asked for — the same authoritative
+   * reading `saveFields` gives its own empty list. Pinned alongside the
+   * anti-wipe control in `MetadataService.objectPayloadFieldsMap.test.ts`, so
+   * the two readings of "no fields" stay distinguishable.
+   *
+   * Two readings were declined by the maintainer ruling (2026-08-27), recorded
+   * here so neither returns as a shortcut:
+   *
+   *   - ⛔ **Not a `{}` default.** `{}` parses GREEN, so defaulting would delete
+   *     every field of the object on a save that only meant to rename it —
+   *     trading a loud, harmless 422 for silent data loss. `toObjectPayload`
+   *     still omits the key for an input that supplies nothing, and that branch
+   *     is still reachable: this class is public API through
+   *     `useMetadataService`, so a JavaScript consumer, or one that casts past
+   *     the types, can arrive there. What it must get is the 422, not the wipe.
+   *   - ⛔ **Not fetch-and-merge.** `saveFields` GETs the current document and
+   *     spreads it, and an object save could preserve the stored `fields` the
+   *     same way — but that builds capability for a path with zero measured
+   *     pull and makes this parameter redundant, which then wants retiring on
+   *     its own terms (ADR-0049 shape).
    */
-  async saveObject(obj: ObjectDefinition, existingFields?: FieldMetadataPayload[]): Promise<void> {
+  async saveObject(obj: ObjectDefinition, existingFields: FieldMetadataPayload[]): Promise<void> {
     const client = this.adapter.getClient();
     const payload = toObjectPayload(obj, existingFields);
     await client.meta.saveItem('object', obj.name, payload);