Skip to content

Commit 82cb6e8

Browse files
os-zhuangclaude
andauthored
fix(metadata-protocol): make the destructive-change remedy clause face-aware — stop prescribing ?force=true on the duplicate door (#11015) (#11099)
* fix(metadata-protocol): make the destructive-change remedy clause face-aware (#11015) `saveMetaItem`'s Phase 3a-destructive refusal ended every message with `— re-submit with ?force=true to proceed.` The refusal is raised in one place and quoted onto whatever response the caller's catch builds, so that sentence went out on every face — including `POST /packages/:id/duplicate`, which accepts no `force` in the query string or the body, and whose `duplicatePackage` request type has no `force` field for the internal `saveMetaItem` call to carry. The clause is now rendered per face. `duplicatePackage` states the face on its internal call — server-stated, exactly as `source` already is — and gets the remedies that exist there: a free target namespace, or reconciling the collision first. Faces that state no door keep the previous wording byte for byte. Repairs the CLAUSE, not the door: no `force` is added to the duplicate route. #10886's verdict is untouched — the per-field findings prose stays, because `failed[].error` is its sole carrier on that face. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RfyXxZ2WPjcjhuXpiQQc3y * docs(metadata-protocol): name #11095 as the card holding the two unrepaired faces (#11015) The docblock and the face-inventory header both recorded that inventory rows 2 and 3 carry the same wrong remedy and were knowingly left. Point them at the issue that now holds that measurement. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RfyXxZ2WPjcjhuXpiQQc3y --------- Co-authored-by: os-zhuang <noreply@anthropic.com>
1 parent 24043c2 commit 82cb6e8

3 files changed

Lines changed: 267 additions & 10 deletions

File tree

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,53 @@
1+
---
2+
"@objectstack/metadata-protocol": patch
3+
---
4+
5+
fix(metadata-protocol): stop prescribing `?force=true` on the duplicate door, which accepts no `force` (#11015)
6+
7+
`saveMetaItem`'s Phase 3a-destructive refusal ended every message with
8+
`— re-submit with ?force=true to proceed.` The refusal is raised in one place
9+
and quoted onto whatever response the caller's catch builds, so that one
10+
sentence went out on every face that reaches the gate — including
11+
`POST /packages/:id/duplicate`, which has no `force` to set.
12+
13+
Measured: the duplicate route accepts `targetPackageId`, `targetName`,
14+
`targetNamespace`, `organizationId` and `actor` — no `force` in the query
15+
string or the body — and `duplicatePackage`'s own request type has no `force`
16+
field either, so its internal `saveMetaItem` call cannot carry one. The gate is
17+
reached on the ordinary duplicate-**again** workflow, where the target
18+
namespace already holds the renamed object from an earlier duplicate; the copy
19+
is refused and the refusal is reported as data on a `200`:
20+
21+
```
22+
"error": "[destructive_change] object/crm2_task would drop or transform existing
23+
data: Field 'b' removed — … — re-submit with ?force=true to proceed."
24+
```
25+
26+
A caller who does what that sentence says gets the identical refusal back. The
27+
remedies that do exist on that face — duplicate into a target namespace that is
28+
free, or reconcile the colliding object first — were never stated.
29+
30+
The clause is now rendered per face. The duplicate door says:
31+
32+
```
33+
… — this copy cannot be forced: the duplicate door accepts no `force`.
34+
Duplicate into a target namespace that does not already hold 'crm2_task', or
35+
reconcile that item with the source first.
36+
```
37+
38+
Three narrowings, each pinned:
39+
40+
- **The clause is repaired, not the door.** No `force` parameter is added to
41+
`POST /packages/:id/duplicate`; that would widen a public surface and is a
42+
contract decision, not a message fix. Which face is being served is stated by
43+
the server on the internal call, exactly as `source` already is — a caller
44+
cannot smuggle one in.
45+
- **Nothing else in the message moved.** #10886 measured that
46+
`duplicatePackage`'s `failed[].error` is the sole carrier of the per-field
47+
destructive findings, so the findings prose stays verbatim. Only the trailing
48+
remedy sentence is face-dependent.
49+
- **No accept/reject behaviour changed.** The copy is still refused, still
50+
reported as `failed[]` data on the `200`, still counted. Faces that state no
51+
door — the single-segment REST `PUT /api/v1/meta/:type/:name`, where
52+
`?force=true` is a real query parameter the route threads — keep the previous
53+
wording byte for byte.

packages/metadata-protocol/src/protocol.destructive-409-face-inventory.test.ts

Lines changed: 138 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,23 @@
8181
* the ablation therefore needs no rebuild, and its RED result is what rules
8282
* out the stale-artifact false green.
8383
*
84+
* ## [#11015] The same inventory, read one column further left
85+
*
86+
* The `force` column above is not decoration: it says which faces can lift
87+
* this refusal, and only ROW 1 can. Rows 2, 3 and 6 all reach the gate with no
88+
* way to set `force` — rows 2 and 3 because their routes never thread the
89+
* parameter, row 6 because `duplicatePackage` has no `force` field at all —
90+
* yet every one of them used to be handed the sentence `re-submit with
91+
* ?force=true to proceed.` A caller who does what it says gets the identical
92+
* refusal back.
93+
*
94+
* #11015 repairs the clause on ROW 6, where a genuinely different remedy
95+
* exists to prescribe (a free target namespace, or reconciling the collision).
96+
* Rows 2 and 3 are left as measured and filed as #11095: the honest repair for
97+
* a `PUT` that cannot acknowledge a risk may be to thread `force` on those
98+
* routes, which is a contract decision and not a message fix. Section 4 pins
99+
* row 6; section 1's remedy guard pins that row 1's wording is untouched.
100+
*
84101
* ⛔ Never a bare `toThrow()` here. `duplicatePackage` does not throw, it
85102
* REPORTS, and what the report says IS the defect; and for the throw itself
86103
* the minimum assertion is `code` + `status` (ADR-0112 envelope), with the
@@ -206,8 +223,19 @@ async function destructiveRefusal(): Promise<any> {
206223
throw new Error('expected saveMetaItem to refuse the destructive change');
207224
}
208225

209-
/** The remedy sentence that must survive ANY future trim (#10886 non-effect). */
210-
const REMEDY = 're-submit with ?force=true to proceed.';
226+
/**
227+
* The remedy sentence that must survive ANY future trim (#10886 non-effect),
228+
* as the ordinary REST `PUT` door renders it. `?force=true` is a real query
229+
* parameter THERE — the route reads it and threads it into the request.
230+
*/
231+
const PUT_REMEDY = 're-submit with ?force=true to proceed.';
232+
/**
233+
* [#11015] …and as the DUPLICATE door renders it, which is a different
234+
* sentence because `?force=true` is not a thing a caller can set on that face.
235+
* See section 4 — the remedy stays, the mechanism it names becomes one that
236+
* exists.
237+
*/
238+
const DUPLICATE_REMEDY_HEAD = 'this copy cannot be forced';
211239
/** One finding's prose, as `detectDestructiveObjectChanges` words it. */
212240
const FINDING_PROSE = "Field 'b' removed — existing data in this column will become inaccessible.";
213241

@@ -243,7 +271,10 @@ describe('[#10886] the 409 renders its findings into the message AND attaches th
243271
// refusal, this is a risk-ACKNOWLEDGEMENT flow: the remedy is the
244272
// whole point, it is not one of the `issues`, and nothing else on any
245273
// face carries it.
246-
expect(err.message).toContain(REMEDY);
274+
// No `writeFace` on this request — the ordinary REST/Studio save, the
275+
// one door where `?force=true` is real. [#11015] made this clause
276+
// face-aware; this default is byte-identical to what it always said.
277+
expect(err.message).toContain(PUT_REMEDY);
247278
const wire = JSON.stringify(err.issues);
248279
expect(wire).not.toContain('force=true');
249280
});
@@ -317,18 +348,119 @@ describe('[#10886] [GUARD] `duplicatePackage`’s `failed[].error` is the SOLE c
317348
expect(entry.issues).toBeUndefined();
318349
});
319350

320-
it('⛔ carries the `?force=true` remedy, on a response with no other channel for it', async () => {
351+
it('⛔ carries the remedy, on a response with no other channel for it', async () => {
321352
const { protocol } = duplicateIntoOccupiedNamespace();
322353

323354
const r = await protocol.duplicatePackage({
324355
sourcePackageId: PKG, targetPackageId: TARGET_PKG,
325356
});
326357

327-
expect(r.failed[0].error).toContain(REMEDY);
358+
// ⚠️ [#11015] This assertion USED to read `toContain(REMEDY)` with
359+
// REMEDY = the `?force=true` sentence, and it passed — because the
360+
// producer rendered that sentence on every face. It was pinning the
361+
// defect: this door accepts no `force`, so the prescription it quoted
362+
// was unactionable. Replaced rather than re-spelled, because what it
363+
// asserted stopped being true of a correct producer. What #10886 put
364+
// it here to protect is unchanged and still asserted: SOME remedy
365+
// reaches the caller through this string and through nothing else.
366+
expect(r.failed[0].error).toContain(DUPLICATE_REMEDY_HEAD);
328367
// The whole response, not just the entry: nothing anywhere else on it
329368
// states the remedy or the findings.
330369
const wire = JSON.stringify({ ...r, failed: r.failed.map((f: any) => ({ ...f, error: '' })) });
331-
expect(wire).not.toContain('force=true');
370+
expect(wire).not.toContain('cannot be forced');
332371
expect(wire).not.toContain('inaccessible');
333372
});
334373
});
374+
375+
// ═══════════════════════════════════════════════════════════════════════════
376+
// 4. [#11015] [GUARD] The remedy names a mechanism THIS face actually has
377+
// ═══════════════════════════════════════════════════════════════════════════
378+
379+
describe('[#11015] [GUARD] the destructive remedy clause is face-aware', () => {
380+
/** Same reachability fixture as section 3 — the duplicate-AGAIN workflow. */
381+
const duplicateIntoOccupiedNamespace = () => makeKernel({
382+
seed: [
383+
objectRow('crm_task', ['a']),
384+
objectRow('crm2_task', ['a', 'b', 'c', 'd'], TARGET_PKG),
385+
],
386+
});
387+
388+
const duplicateFailure = async (extra: Record<string, unknown> = {}) => {
389+
const { protocol } = duplicateIntoOccupiedNamespace();
390+
const r = await protocol.duplicatePackage({
391+
sourcePackageId: PKG, targetPackageId: TARGET_PKG, ...extra,
392+
});
393+
return r;
394+
};
395+
396+
it('⛔ the duplicate face never prescribes `force` — the door accepts none', async () => {
397+
const r = await duplicateFailure();
398+
399+
// The defect, stated as the assertion that would have failed before
400+
// the fix. Not `not.toContain(PUT_REMEDY)` alone: the substring that
401+
// must be gone is the MECHANISM NAME, because a caller reading it goes
402+
// looking for a parameter that does not exist on this door.
403+
expect(r.failed[0].error).not.toContain('force=true');
404+
expect(r.failed[0].error).not.toContain(PUT_REMEDY);
405+
});
406+
407+
it('prescribes the remedies that DO exist on this face, and names the collision', async () => {
408+
const r = await duplicateFailure();
409+
const error: string = r.failed[0].error;
410+
411+
// Both real remedies, in the caller's own vocabulary — `targetNamespace`
412+
// is a parameter this door genuinely accepts.
413+
expect(error).toContain('target namespace');
414+
expect(error).toContain('reconcile');
415+
// …and WHICH item collides, which is the copy's re-namespaced name
416+
// (`crm_task` → `crm2_task`), not the source row's.
417+
expect(error).toContain('crm2_task');
418+
});
419+
420+
it('[#10886 non-effect] the per-field findings prose is still there, untrimmed', async () => {
421+
const r = await duplicateFailure();
422+
423+
// ⛔ This card repaired the remedy clause ONLY. #10886's verdict — the
424+
// findings prose stays, because `failed[].error` is its sole carrier on
425+
// this face — is untouched, and this is the assertion that says so.
426+
expect(r.failed[0].error).toContain(FINDING_PROSE);
427+
expect(r.failed[0].error).toContain('[destructive_change]');
428+
});
429+
430+
it('the refusal still REFUSES — this is a message repair, not a behaviour one', async () => {
431+
const r = await duplicateFailure();
432+
433+
// Clause-② line: no accept/reject behaviour moved. The copy is still
434+
// rejected, still reported as data on the 200, still counted.
435+
expect(r.success).toBe(false);
436+
expect(r.copiedCount).toBe(0);
437+
expect(r.failedCount).toBe(1);
438+
expect(r.copied).toEqual([]);
439+
});
440+
441+
it('⛔ the face is stated by the SERVER — a caller cannot smuggle one in', async () => {
442+
// The duplicate route builds `duplicatePackage`'s request field by
443+
// field and this method hard-codes the face on its internal
444+
// `saveMetaItem` call, so neither a `force` nor a `writeFace` on the
445+
// caller's request can reach the gate. Asserted from the OUTSIDE
446+
// rather than by reading the type, because the type is what a future
447+
// edit would widen: if adding `force` to this door ever becomes the
448+
// decision, this test is the one that has to be rewritten deliberately
449+
// instead of quietly starting to pass.
450+
const smuggled = await duplicateFailure({ force: true, writeFace: undefined });
451+
452+
expect(smuggled.failedCount).toBe(1);
453+
expect(smuggled.failed[0].error).toContain(DUPLICATE_REMEDY_HEAD);
454+
expect(smuggled.failed[0].error).not.toContain('force=true');
455+
});
456+
457+
it('the OTHER faces keep the `?force=true` wording — a switch, not a global delete', async () => {
458+
// Row 1 of the inventory, driven at the producer with no face stated.
459+
const err = await destructiveRefusal();
460+
461+
expect(err.code).toBe('DESTRUCTIVE_CHANGE');
462+
expect(err.status).toBe(409);
463+
expect(err.message).toContain(PUT_REMEDY);
464+
expect(err.message).not.toContain(DUPLICATE_REMEDY_HEAD);
465+
});
466+
});

packages/metadata-protocol/src/protocol.ts

Lines changed: 76 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3202,6 +3202,64 @@ function detectDestructiveObjectChanges(prev: any, next: any): Array<{
32023202
return issues;
32033203
}
32043204

3205+
/**
3206+
* [#11015] The remedy clause the Phase 3a-destructive refusal ends with — one
3207+
* sentence per FACE, because the mechanism that lifts the refusal is not the
3208+
* same on every door that raises it.
3209+
*
3210+
* The refusal is raised in ONE place ({@link ObjectStackProtocolImplementation.saveMetaItem}'s
3211+
* Phase 3a-destructive gate) and is quoted onto whatever response the caller's
3212+
* catch builds, so the clause used to read `re-submit with ?force=true to
3213+
* proceed.` on every face. That is correct prose for exactly one of them —
3214+
* `PUT /api/v1/meta/:type/:name`, whose route reads `?force` and threads it
3215+
* into the request — and a caller on any OTHER face who does what the sentence
3216+
* says gets the identical refusal back.
3217+
*
3218+
* MEASURED on the face this card was filed on: `POST /packages/:id/duplicate`
3219+
* accepts `targetPackageId`, `targetName`, `targetNamespace`, `organizationId`
3220+
* and `actor` — no `force`, in the query string or the body — and
3221+
* {@link ObjectStackProtocolImplementation.duplicatePackage}'s own request type
3222+
* has no `force` field either, so its internal `saveMetaItem` call cannot carry
3223+
* one. There is nothing on that door for the caller to set.
3224+
*
3225+
* ⛔ What is repaired is the CLAUSE, not the door. Giving the duplicate route a
3226+
* `force` would widen a public surface and is a contract decision, deliberately
3227+
* NOT taken here. Nor is the clause deleted on that face: #10886 measured that
3228+
* `duplicatePackage`'s `failed[].error` is the SOLE carrier of this
3229+
* prescription, so deleting the remedy there deletes it from the wire outright.
3230+
* Each face therefore states the remedy it actually has.
3231+
*
3232+
* ⚠️ An absent `face` renders the `?force=true` wording, byte-identical to what
3233+
* every face carried before. That default is right on the single-segment REST
3234+
* `PUT` — but `protocol.destructive-409-face-inventory.test.ts` inventories two
3235+
* further doors that reach this gate and never thread `force` either
3236+
* (`@objectstack/rest`'s compound-name `PUT /meta/:type/:a/:b`, and
3237+
* `@objectstack/runtime`'s dispatcher `PUT /meta`). Those are wrong for the
3238+
* same reason and are deliberately NOT repaired here: unlike the duplicate
3239+
* gesture, which has a genuine collision-free alternative to prescribe, the
3240+
* right repair for a `PUT` that cannot acknowledge a risk may well be to thread
3241+
* `force` on those routes — a contract question, filed as #11095 rather than
3242+
* guessed at. Adding a face value here is one of the two candidate repairs it
3243+
* weighs.
3244+
*/
3245+
function destructiveChangeRemedy(
3246+
face: 'package-duplicate' | undefined,
3247+
name: string,
3248+
): string {
3249+
switch (face) {
3250+
case 'package-duplicate':
3251+
// The duplicate-AGAIN workflow: the target namespace already holds
3252+
// the renamed item this copy is about to overwrite. Both remedies
3253+
// are things the caller can actually do on THIS door — choose a
3254+
// free target namespace, or make the collision non-destructive.
3255+
return `this copy cannot be forced: the duplicate door accepts no \`force\`. `
3256+
+ `Duplicate into a target namespace that does not already hold '${name}', `
3257+
+ `or reconcile that item with the source first.`;
3258+
default:
3259+
return 're-submit with ?force=true to proceed.';
3260+
}
3261+
}
3262+
32053263
/**
32063264
* Result of projecting a published metadata body into its data-plane
32073265
* representation. `success:false` with an `error` is the surfaced-not-thrown
@@ -12847,7 +12905,7 @@ export class ObjectStackProtocolImplementation implements
1284712905
}
1284812906
}
1284912907

12850-
async saveMetaItem(request: { type: string, name: string, item?: any, organizationId?: string, parentVersion?: string | null, actor?: string, force?: boolean, mode?: 'draft' | 'publish', packageId?: string | null, source?: string }) {
12908+
async saveMetaItem(request: { type: string, name: string, item?: any, organizationId?: string, parentVersion?: string | null, actor?: string, force?: boolean, mode?: 'draft' | 'publish', packageId?: string | null, source?: string, writeFace?: 'package-duplicate' }) {
1285112909
// [#8818] The ADR-0112 envelope this refusal always owed. Every OTHER
1285212910
// refusal in this method declares `code` AND `status`
1285312911
// (`NOT_OVERRIDABLE`/403, `NOT_CREATABLE`/403, `ITEM_LOCKED`/403,
@@ -13198,16 +13256,22 @@ export class ObjectStackProtocolImplementation implements
1319813256
// `protocol.destructive-409-face-inventory.test.ts`
1319913257
// carries the whole inventory and pins this face.
1320013258
//
13201-
// ⛔ Whatever else a future trim does, the `?force=true`
13202-
// remedy below must survive it: this is a
13259+
// ⛔ Whatever else a future trim does, the remedy
13260+
// clause below must survive it: this is a
1320313261
// risk-acknowledgement refusal, not a validation one,
1320413262
// and no structured channel on any face carries the
1320513263
// remedy.
13264+
//
13265+
// [#11015] Which remedy that IS depends on the face —
13266+
// `?force=true` names a query parameter only the
13267+
// single-segment REST `PUT` reads, and prescribing it
13268+
// to a caller who has no way to set it sends them in a
13269+
// circle. See {@link destructiveChangeRemedy}.
1320613270
const summary = issues.slice(0, 3).map((i) => i.message).join('; ');
1320713271
const err = new Error(
1320813272
`[destructive_change] ${request.type}/${request.name} would drop or transform existing data: ${summary}`
1320913273
+ (issues.length > 3 ? ` (+${issues.length - 3} more)` : '')
13210-
+ ` — re-submit with ?force=true to proceed.`
13274+
+ ` — ${destructiveChangeRemedy(request.writeFace, request.name)}`
1321113275
);
1321213276
(err as any).code = 'DESTRUCTIVE_CHANGE';
1321313277
(err as any).status = 409;
@@ -16847,6 +16911,14 @@ export class ObjectStackProtocolImplementation implements
1684716911
item: rewritten,
1684816912
mode: 'publish',
1684916913
packageId: request.targetPackageId,
16914+
// [#11015] Which door the refusal below will be prescribing
16915+
// a remedy FOR. Stated by the server, never by the caller —
16916+
// `duplicatePackage`'s own request type has no such field,
16917+
// exactly as `source` is server-stated one gate down. The
16918+
// Phase 3a-destructive gate reaches this call on the
16919+
// duplicate-AGAIN workflow, and its `?force=true` default
16920+
// would name a parameter this door does not accept.
16921+
writeFace: 'package-duplicate',
1685016922
...(copyOrgId ? { organizationId: copyOrgId } : {}),
1685116923
...(request.actor ? { actor: request.actor } : {}),
1685216924
});

0 commit comments

Comments
 (0)