Skip to content

Commit 33a45cc

Browse files
fix(metadata-protocol): removing a field that stores nothing is not a destructive change (#24479)
Fixes #24411 Clause-②: yes (widening) (the save door newly accepts, without `force`, a removal it refused; owes the seat's `TIER_DEFAULT` review of record before the queue) Dispatched by the `domain:engine` seat 1 PM (session `session_01JtSQQ3EWsbNPzyYMug1tXy`, claim comment 6259474942). Dev session: `session_01RaFonyZMJU1FzgAT78bA47`. ## What changed `detectDestructiveObjectChanges` in `packages/metadata-protocol/src/protocol.ts` (the object save's Phase 3a-destructive diff) now reports a finding only when the field holds stored data. "Stored" is the spec's storage predicate `isVirtualSearchField` (`SEARCH_VIRTUAL_TYPES`, exactly `formula` today), read from the field's own declaration. It is the predicate the served sortability projection (`packages/spec/src/api/sortability.zod.ts`) already reads, and `protocol.ts` already imported it. No name list or type list is kept in the detector. - `field_removed`: skipped when the removed field is unstored. - `field_type_change`: judged by what the stored side loses. An unstored previous field (a formula) held no values, so retyping it into any kind is not destructive. Retyping a stored field into a formula is still refused, because the stored side loses its column. - `field_required_no_default`: unchanged. It is not a storage-loss finding (see Acceptance notes). - No change to the `force` mechanism, to the message of a true removal, or to any error code. ## Measured (M, booted composition) Booted with `bootStack` (`@objectstack/verify`, sqlite-wasm) on a fixture app. Object `dfu_invoice` was saved at runtime through `PUT /api/v1/meta/object/dfu_invoice` with `name`/`notes` (text), `amount` (number), `doubled` (formula) and `reference` (autonumber), plus one record. Pin: `packages/qa/dogfood/test/meta-object-unstored-field-removal.dogfood.test.ts`. | case | before (`main` `fa26308bb0`) | after | |:--|:--|:--| | remove `doubled` (formula), no `force` | `409 DESTRUCTIVE_CHANGE`, "Field 'doubled' removed — existing data in this column will become inaccessible. — re-submit with ?force=true to proceed." | `200`. `GET /data/dfu_invoice/:id` reads back `name: 'INV-1'`, `notes: 'keep me'`, `amount: 21`, `reference: '0001'` unchanged | | remove `notes` (stored text), no `force` | `409 DESTRUCTIVE_CHANGE`, issues `[{code: 'field_removed', field: 'notes', message: "Field 'notes' removed — existing data in this column will become inaccessible."}]` | identical `409`, same issue and message | | remove `reference` (autonumber: engine-computed but stored) | `409 DESTRUCTIVE_CHANGE` | `409 DESTRUCTIVE_CHANGE` (unchanged) | The predicate names only one unstored kind (`formula`). `summary` and `autonumber` are stored columns: the sortability docblock records them as "deliberately NOT members". So the "one more kind" reading is `autonumber`, measured as a stored control that still answers 409. This confirms the axis is storage, not `COMPUTED_VALUE_TYPES`. Unit pins (`protocol.destructive-gate-reachable-types.test.ts` §4, reusing that file's existing engine double, so no new fake engine): - stored-text removal control: exact issue list, nothing written; - formula removal resolves and persists; - autonumber removal refused; - formula + text removed together reports `notes` alone; - formula → number resolves; - text → formula refused with `field_type_change`. Before the fix, the three unstored cases were red and the stored controls green (`Tests 3 failed | 13 passed (16)`). ### Ablation (one-shot, not kept) - **Mutation:** `node scripts/ablation-replace.mjs --delete` removed the line `if (!isStored(prevFields[fname])) continue;` (anchor x1 → x0, blob `767873d0` → `fb38f325`). - **Rebuild and preflight:** `@objectstack/metadata-protocol` rebuilt; `ablation-dist-preflight --absent` reported the marker absent from all 24 built files. - **Result, red:** unit `2 failed | 14 passed` (formula removal, formula+text). Dogfood `1 failed | 2 passed`: the formula removal answered the old 409. - **Restore:** blob back to `767873d0`, equal to HEAD, and `git diff HEAD` empty. After a rebuild, the preflight reported the marker present in 2 built files and the tree clean. Unit 16/16 and dogfood 3/3 green. ## Verification - `pnpm --filter @objectstack/metadata-protocol test`: `Test Files 246 passed | 3 skipped`, `Tests 20661 passed | 19 skipped` (exit 0, pre-merge head `fb06fda4c`). - `pnpm --filter @objectstack/metadata-protocol typecheck`: exit 0. `--listFiles` includes the edited test file. - `pnpm --filter @objectstack/dogfood typecheck`: exit 0. `--listFiles` includes the new pin. - Gates, first pass: `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` (no paths) derived 71 commands on `fb06fda4c`. All 71 were run, each exit captured before any pipe: - 70 exited 0. - `pnpm check:dual-build-cjs-loads` first exited 3 (PREREQUISITE NOT MET: 8 unbuilt packages). Those were built, and the re-run exited 0 ("111 published require entry point(s) across 69 package(s) load"). - `--ran` reconciliation, with codes recorded: "71 derived, 71 run, 0 NOT-MEASURED, 0 UNRUN … a DERIVED zero". - The changeset-only follow-up commit re-ran `check-adr-0087-registration`, `check-changeset-no-major`, `check-empty-changeset`, `check:nul-bytes`, `check:issue-citations` and `check:doc-authoring`: all exit 0. - `origin/main` (`ec951e29e`) was merged cleanly before this PR opened. On the merge head `3174dbc00`, every step exited 0: - `pnpm install --frozen-lockfile`; - `pnpm --filter '@objectstack/dogfood^...' build`; - `pnpm --filter @objectstack/spec check:generated`; - both packages' typechecks; - both pins. - The gate union was re-derived on `3174dbc00` (the same 71 commands) and re-run. All 71 exited 0, `check:dual-build-cjs-loads` included, and `--ran` reconciled "71 derived, 71 run, 0 NOT-MEASURED, 0 UNRUN". ## Pin sweep `git grep` for `field_removed`, `DESTRUCTIVE_CHANGE` and "will become inaccessible" across tests, docs, ADRs and the checklist found no pin that asserts a formula or unstored field removal is destructive. Every existing `field_removed` pin removes a `text` field, and those still hold. No hand-written doc states the old behaviour. ADR-0027's table names `field_removed` generically and still holds. ## Acceptance notes - `field_required_no_default` on a formula is left as is. Its claim is about write validation of rows, not stored data becoming unreachable, and nothing here measured whether required-validation reaches a formula. That is outside this card. - The column planner (`driver-sql` `fieldHasColumn`, `driver-turso`) still spells `type === 'formula'` locally instead of calling the spec predicate. It agrees with the predicate today. This is noted as observed drift risk only, not filed (no carrier). - The claim's pin "a type change between two unstored kinds is not destructive" cannot be built today, because the predicate names one unstored kind. It is pinned as formula → stored (not destructive) and stored → formula (destructive). Once the predicate gains a second kind, the same `isStored(prevField)` check covers it with no edit. --- _Generated by [Claude Code](https://claude.ai/code/session_01RaFonyZMJU1FzgAT78bA47)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 3b086e7 commit 33a45cc

4 files changed

Lines changed: 261 additions & 1 deletion

File tree

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
---
2+
'@objectstack/metadata-protocol': minor
3+
---
4+
5+
fix(metadata-protocol): removing a field that stores nothing is not a destructive change
6+
7+
Clause-②: yes (widening) (the save door newly accepts, without `force`, a removal it refused; owes the seat's `TIER_DEFAULT` review of record before the queue)
8+
9+
- **What changed.** The object save's safety diff (`PUT /api/v1/meta/object/:name`, and every other door that reaches `saveMetaItem`'s destructive gate) now judges each finding by whether the field is stored, using the spec's storage predicate `isVirtualSearchField` (`SEARCH_VIRTUAL_TYPES`) on the field's own declaration — the predicate the sortability projection already reads. A `formula` field is computed on read and has no column (ADR-0079 addendum, item 9), so:
10+
- removing a `formula` field saves `200` without `?force=true` (it answered `409 DESTRUCTIVE_CHANGE` with "existing data in this column will become inaccessible");
11+
- retyping a `formula` field into a stored kind saves without `force`: the new column starts empty, and no stored value is lost.
12+
- **Not changed.** Removing a stored field, including an engine-computed but stored one (`autonumber`, `summary`), still answers `409 DESTRUCTIVE_CHANGE` with the same `field_removed` finding and message. Retyping a stored field into a `formula` still answers `409` with `field_type_change`. The `force` mechanism, the messages and the error codes are unchanged.
13+
- **Who notices.** An author or an agent that removes a formula field no longer needs `force=true` for it, so `force=true` stays reserved for a real loss of stored data.

‎packages/metadata-protocol/src/protocol.destructive-gate-reachable-types.test.ts‎

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -504,3 +504,114 @@ describe('[#11014 §3] the double fault the trimmed limb used to catch', () => {
504504
expect(FieldSchema.safeParse(stored).success).toBe(false);
505505
});
506506
});
507+
508+
// ═══════════════════════════════════════════════════════════════════════════
509+
// 4. [#24411] A field that STORES NOTHING loses nothing — judged by the spec's
510+
// storage predicate, never by a type list kept beside the detector
511+
// ═══════════════════════════════════════════════════════════════════════════
512+
513+
describe('[#24411 §4] the destructive diff reads the field\'s own storage declaration', () => {
514+
/**
515+
* The gate's findings each say what stored data a save makes unreachable.
516+
* A `formula` is computed on read and materialises no column (ADR-0079
517+
* addendum, item 9), so removing one, or retyping one into a stored kind,
518+
* loses nothing — and a 409 there trains authors to send `force=true` by
519+
* reflex, which silences the true finding on a stored field too.
520+
*
521+
* "Stored" is `isVirtualSearchField` (`SEARCH_VIRTUAL_TYPES`), the
522+
* predicate the sortability projection reads. `autonumber` is the
523+
* contrast that keeps the axis honest: engine-computed, but stored in a
524+
* column, so its removal is still a data-loss event.
525+
*/
526+
const OBJ = 'crm_invoice';
527+
const STORED = {
528+
name: { name: 'name', type: 'text' },
529+
notes: { name: 'notes', type: 'text' },
530+
amount: { name: 'amount', type: 'number' },
531+
reference: { name: 'reference', type: 'autonumber' },
532+
};
533+
const DOUBLED = { name: 'doubled', type: 'formula', expression: 'record.amount * 2' };
534+
const seed = () => row({
535+
type: 'object',
536+
name: OBJ,
537+
metadata: JSON.stringify({ name: OBJ, label: OBJ, fields: { ...STORED, doubled: DOUBLED } }),
538+
});
539+
const saveFields = (protocol: any, fields: Record<string, unknown>) =>
540+
save(protocol, { type: 'object', name: OBJ, item: { name: OBJ, label: OBJ, sharingModel: 'private', fields } });
541+
const withoutField = (drop: keyof typeof STORED) =>
542+
Object.fromEntries(Object.entries({ ...STORED, doubled: DOUBLED }).filter(([k]) => k !== drop));
543+
544+
it('CONTROL: a stored `text` removal is still refused, with the same finding and message', async () => {
545+
const { protocol, writes } = makeKernel([seed()]);
546+
547+
const r = await saveFields(protocol, withoutField('notes'));
548+
549+
expect(r.outcome).toBe('threw');
550+
const err = (r as { error: any }).error;
551+
expect(err.code).toBe('DESTRUCTIVE_CHANGE');
552+
expect(err.status).toBe(409);
553+
expect(err.issues).toEqual([{
554+
code: 'field_removed',
555+
field: 'notes',
556+
message: "Field 'notes' removed — existing data in this column will become inaccessible.",
557+
}]);
558+
expect(metadataWrites(writes)).toEqual([]);
559+
});
560+
561+
it('a `formula` removal saves without `force`', async () => {
562+
const { protocol, writes } = makeKernel([seed()]);
563+
564+
const r = await saveFields(protocol, { ...STORED });
565+
566+
expect(r.outcome, JSON.stringify((r as any).error?.issues)).toBe('resolved');
567+
expect(metadataWrites(writes).length).toBeGreaterThan(0);
568+
});
569+
570+
it('an `autonumber` removal is still refused: engine-computed, but stored', async () => {
571+
const { protocol } = makeKernel([seed()]);
572+
573+
const r = await saveFields(protocol, withoutField('reference'));
574+
575+
expect(r.outcome).toBe('threw');
576+
const err = (r as { error: any }).error;
577+
expect(err.code).toBe('DESTRUCTIVE_CHANGE');
578+
expect(err.status).toBe(409);
579+
expect(err.issues).toEqual([expect.objectContaining({ code: 'field_removed', field: 'reference' })]);
580+
});
581+
582+
it('removing a formula and a stored field together reports the stored one alone', async () => {
583+
const { protocol } = makeKernel([seed()]);
584+
585+
const { notes: _notes, ...rest } = STORED;
586+
const r = await saveFields(protocol, rest);
587+
588+
expect(r.outcome).toBe('threw');
589+
const err = (r as { error: any }).error;
590+
expect(err.code).toBe('DESTRUCTIVE_CHANGE');
591+
expect(err.issues.map((i: any) => i.field)).toEqual(['notes']);
592+
});
593+
594+
it('retyping a formula into a stored kind loses nothing: the stored side starts empty', async () => {
595+
const { protocol } = makeKernel([seed()]);
596+
597+
const r = await saveFields(protocol, { ...STORED, doubled: { name: 'doubled', type: 'number' } });
598+
599+
expect(r.outcome, JSON.stringify((r as any).error?.issues)).toBe('resolved');
600+
});
601+
602+
it('retyping a stored field into a formula is still refused: the stored side loses its column', async () => {
603+
const { protocol } = makeKernel([seed()]);
604+
605+
const r = await saveFields(protocol, {
606+
...STORED,
607+
notes: { name: 'notes', type: 'formula', expression: 'record.name' },
608+
doubled: DOUBLED,
609+
});
610+
611+
expect(r.outcome).toBe('threw');
612+
const err = (r as { error: any }).error;
613+
expect(err.code).toBe('DESTRUCTIVE_CHANGE');
614+
expect(err.status).toBe(409);
615+
expect(err.issues).toEqual([expect.objectContaining({ code: 'field_type_change', field: 'notes' })]);
616+
});
617+
});

‎packages/metadata-protocol/src/protocol.ts‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5689,12 +5689,22 @@ function detectDestructiveObjectChanges(prev: any, next: any): Array<{
56895689

56905690
const issues: Array<{ code: string; field?: string; message: string }> = [];
56915691

5692+
// [#24411] Every finding below names STORED data a save makes unreachable,
5693+
// so a field with no stored column (a `formula`, computed on read —
5694+
// ADR-0079 addendum, item 9) loses nothing when it is removed or retyped.
5695+
// "Stored" is the spec's storage predicate — the one the sortability
5696+
// projection reads — judged from the field's own declaration, ⛔ never a
5697+
// type list kept here. A false finding trains callers to send `force=true`
5698+
// by reflex, which silences the true one on a stored field.
5699+
const isStored = (field: SearchFieldMeta | undefined): boolean => !isVirtualSearchField(field);
5700+
56925701
// Removed fields — silently dropping a column is a data-loss event.
56935702
for (const fname of Object.keys(prevFields)) {
56945703
// Skip system fields — those are managed by applySystemFields and
56955704
// re-injected on every registerObject call; they will look "removed"
56965705
// in any user-supplied overlay.
56975706
if (prevFields[fname]?.system) continue;
5707+
if (!isStored(prevFields[fname])) continue;
56985708
if (!(fname in nextFields)) {
56995709
issues.push({
57005710
code: 'field_removed',
@@ -5718,7 +5728,9 @@ function detectDestructiveObjectChanges(prev: any, next: any): Array<{
57185728
if (!prevField) continue; // brand-new field — non-destructive
57195729
const prevType = prevField.type;
57205730
const nextType = nextField.type;
5721-
if (prevType && nextType && prevType !== nextType) {
5731+
// Judged by what the STORED side loses: an unstored previous field held
5732+
// no values, so retyping it into anything starts from an empty column.
5733+
if (prevType && nextType && prevType !== nextType && isStored(prevField)) {
57225734
const compatible = TYPE_COMPATIBILITY[prevType]?.has(nextType);
57235735
if (!compatible) {
57245736
issues.push({
Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
//
3+
// GOLDEN REGRESSION — removing a field that stores nothing is not a destructive
4+
// change, booted for real and driven through `PUT /api/v1/meta/object/:name`.
5+
//
6+
// The object save's safety diff refuses, with `409 DESTRUCTIVE_CHANGE`, a save
7+
// that drops a field: the column's data becomes unreachable, and only
8+
// `?force=true` lifts the refusal. That is true of a stored field and false of
9+
// a `formula`, which is computed on read and has no column (ADR-0079 addendum,
10+
// item 9). Refusing the formula removal taught authors and agents to send
11+
// `force=true` by reflex — which silences the true warning on a stored field.
12+
//
13+
// "Stored" is the spec's storage predicate (`isVirtualSearchField` /
14+
// `SEARCH_VIRTUAL_TYPES`), the one the sortability projection reads — so the
15+
// cases below are judged by what each field declares:
16+
//
17+
// 1. a `formula` removal saves 200 without `force`, and the stored values of
18+
// the other fields read back unchanged;
19+
// 2. a stored `text` removal still answers 409 DESTRUCTIVE_CHANGE, with the
20+
// same finding and message — the live control that this harness reaches
21+
// the gate at all;
22+
// 3. an `autonumber` removal still answers 409: it is computed by the ENGINE
23+
// but stored in a column, so "computed" is not the axis — storage is.
24+
//
25+
// Boots a fixture stack of its own, so it stays out of `SHARED_SHOWCASE`.
26+
27+
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
28+
import { bootStack, type VerifyStack } from '@objectstack/verify';
29+
import { cel, defineStack } from '@objectstack/spec';
30+
31+
const NAME = 'dfu_invoice';
32+
33+
const stackDef = defineStack({
34+
manifest: {
35+
id: 'com.dogfood.unstored-removal',
36+
namespace: 'dfu',
37+
version: '0.0.0',
38+
type: 'app',
39+
name: 'Unstored Field Removal',
40+
description: 'An empty app; the object under test is saved at runtime through the metadata door.',
41+
},
42+
});
43+
44+
/** Every field the object starts with — each case removes exactly one. */
45+
const FIELDS: Record<string, Record<string, unknown>> = {
46+
name: { type: 'text', label: 'Name' },
47+
notes: { type: 'text', label: 'Notes' },
48+
amount: { type: 'number', label: 'Amount' },
49+
// JSON round-trip: the wire form of the `cel` envelope.
50+
doubled: { type: 'formula', label: 'Doubled', expression: JSON.parse(JSON.stringify(cel`record.amount * 2`)) },
51+
reference: { type: 'autonumber', label: 'Reference' },
52+
};
53+
54+
const definition = (fields: Record<string, Record<string, unknown>>) => ({
55+
name: NAME,
56+
label: 'Invoice',
57+
sharingModel: 'public_read_write',
58+
fields,
59+
});
60+
61+
const without = (drop: string) => Object.fromEntries(Object.entries(FIELDS).filter(([k]) => k !== drop));
62+
63+
describe('dogfood: removing an unstored field through PUT /meta/object/:name', () => {
64+
let stack: VerifyStack;
65+
let token: string;
66+
let recordId: string;
67+
68+
const call = async (method: string, path: string, body?: unknown) => {
69+
const res = await stack.apiAs(token, method, path, body);
70+
const json: any = await res.json().catch(() => ({}));
71+
return { status: res.status, json };
72+
};
73+
74+
75+
beforeAll(async () => {
76+
stack = await bootStack(stackDef);
77+
token = await stack.signIn();
78+
79+
const saved = await call('PUT', `/meta/object/${NAME}`, definition(FIELDS));
80+
expect(saved.status, JSON.stringify(saved.json)).toBe(200);
81+
82+
const created = await call('POST', `/data/${NAME}`, { name: 'INV-1', notes: 'keep me', amount: 21 });
83+
expect(created.status, JSON.stringify(created.json)).toBeLessThan(300);
84+
recordId = String(created.json?.data?.id ?? created.json?.id);
85+
expect(recordId, JSON.stringify(created.json)).toMatch(/\S/);
86+
}, 300_000);
87+
88+
afterAll(async () => {
89+
await stack?.stop?.();
90+
});
91+
92+
it('a stored `text` removal still answers 409 DESTRUCTIVE_CHANGE with today\'s finding (live control)', async () => {
93+
const refused = await call('PUT', `/meta/object/${NAME}`, definition(without('notes')));
94+
expect(refused.status, JSON.stringify(refused.json)).toBe(409);
95+
expect(refused.json?.code, JSON.stringify(refused.json)).toBe('DESTRUCTIVE_CHANGE');
96+
expect(refused.json?.issues).toEqual([{
97+
code: 'field_removed',
98+
field: 'notes',
99+
message: "Field 'notes' removed — existing data in this column will become inaccessible.",
100+
}]);
101+
});
102+
103+
it('an `autonumber` removal still answers 409: engine-computed, but stored in a column', async () => {
104+
const refused = await call('PUT', `/meta/object/${NAME}`, definition(without('reference')));
105+
expect(refused.status, JSON.stringify(refused.json)).toBe(409);
106+
expect(refused.json?.code, JSON.stringify(refused.json)).toBe('DESTRUCTIVE_CHANGE');
107+
expect(refused.json?.issues).toEqual([expect.objectContaining({ code: 'field_removed', field: 'reference' })]);
108+
});
109+
110+
it('a `formula` removal saves 200 without `force`, and the stored fields read back unchanged', async () => {
111+
const saved = await call('PUT', `/meta/object/${NAME}`, definition(without('doubled')));
112+
expect(saved.status, JSON.stringify(saved.json)).toBe(200);
113+
114+
const meta = await call('GET', `/meta/object/${NAME}`);
115+
expect(meta.status, JSON.stringify(meta.json)).toBe(200);
116+
expect(Object.keys(meta.json?.item?.fields ?? {})).not.toContain('doubled');
117+
118+
const read = await call('GET', `/data/${NAME}/${recordId}`);
119+
expect(read.status, JSON.stringify(read.json)).toBe(200);
120+
expect(read.json?.record, JSON.stringify(read.json)).toMatchObject({
121+
name: 'INV-1', notes: 'keep me', amount: 21, reference: expect.stringMatching(/\S/),
122+
});
123+
});
124+
});

0 commit comments

Comments
 (0)