Skip to content

Commit 8226755

Browse files
fix(metadata-protocol): duplicatePackage refuses a copied item whose renamed object reference meets a name it already holds (#23545)
Fixes #23537 Clause-②: no ## What this does `duplicatePackage` (`packages/metadata-protocol/src/protocol.ts`, ADR-0070 D4) promises to rewrite "every intra-package reference". At a property that names an object, its `deepRewrite` renames a name whole: a string, an array entry, or a record KEY. When the source item also names the object the copy is about to be called (`dupdst_customer`, an object outside the package or not yet created) beside the package's own (`dupsrc_customer`), the rename lands on a name the item already holds. Until now the copy merged the two with `failed: []`. Now, in the `duplicatePackage` region only: - **The detection lives where the name is renamed.** `deepRewrite` takes a per-item `collisions` list and records every meeting at a reference site: the path, the name that moved and the name it met. - Record keys: a second source key landing on a key already set in the same record. - Array entries: a renamed entry landing on an entry already in the same array, when the two source entries differ. An entry the source itself repeats is copied as stored. - `renameMap` is one-to-one (a claimed copy name is refused up front since PR #23525), so a meeting is always one renamed name and one name this copy does not rename. - **One call site refuses the item.** Right after `deepRewrite(item, collisions)`, a non-empty list pushes one `failed[]` entry and `continue`s, before `saveMetaItem`. Nothing of the item is written: no item row and no history, commit or audit row (measured below). It is the same "refused, never merged" posture as `rewriteFieldRuleKeys` (PR #23536) and the claimed-name refusal (PR #23525). - **The refusal names the item, the property, both names and the source edit that clears it.** Measured text: > Permission set 'dupsrc_objs' is not copied: its `objects` key 'dupsrc_customer' follows the copied object to 'dupdst_customer', a key it already holds there, which this copy does not rename. A record keeps one entry per key, so the copy would drop one of the two. Remove 'dupsrc_customer' or 'dupdst_customer' on the source, then duplicate again. An item with several collisions names the first and adds "It has N more such collision(s)." - The `duplicatePackage` docblock gains one sentence. No other region of `protocol.ts` is touched. The source-row `where` line that PR #23469 edits is untouched, and `reference-sites.ts` is untouched. ## Zone 2 hypotheses, measured on a real kernel (ObjectKernel + ObjectQLPlugin, memory driver) A one-off probe file (not committed) read the stored rows out of the driver at base `7b64a3d73`, then at head. - **H1 holds.** At base, a source set `objects: { dupsrc_customer: { allowRead }, dupdst_customer: { allowRead, allowEdit, allowDelete } }` copies with `failed: []` as `objects: { dupdst_customer: { allowRead, allowEdit, allowDelete } }`. The last entry in insertion order wins. With the two keys in the other order, the copy is `{ dupdst_customer: { allowRead } }`, so the wide grant is the one dropped. Both orders are pinned. - **H2: the collision reaches every record-keyed or array-valued reference map, and the fix covers them all at once.** See the enumeration below. - **H3: nothing of a refused item is written.** At head, after the refusal, no row in any of `sys_metadata`, `sys_metadata_history`, `sys_metadata_commit` or `sys_metadata_audit` mentions `dupdst_objs`. In the same run, the copied object `dupdst_customer` appears in `sys_metadata`, `sys_metadata_history` and `sys_metadata_audit`, so the scan reads the tables a copy writes. The pin asserts both halves. - **H4 changes an answer, so it is fixed here.** At base, a hook with `object: ['dupsrc_customer', 'dupdst_customer']` copies with `failed: []` as `object: ['dupdst_customer', 'dupdst_customer']`. - Measured through the binder the authored-hook resync calls (`bindHooksToEngine`): the copy's handler ran **twice** per insert on `dupdst_customer`. The source hook ran once per object. - A grant duplicated in a record is merged into one key. A duplicated array entry is not merged: the binder registers each entry, and `normalizeObjects` does not deduplicate. So a non-idempotent handler (a counter, a notification) does twice what its source does. I read that as widening the item's effect, which is the dispatch's condition for fixing it here. It is the same detection in the same walker. Say so if you read the condition otherwise, and the array arm comes out as one hunk. - In this kernel composition, ObjectQLPlugin did not bind hooks saved through `saveMetaItem` (no metadata service, so no engine registration for the source hook or the copy). That is why the double run was measured through the binder directly. ## H2: the reference maps a copied item can carry Method: the derived site index (`REFERENCE_SITES.byTarget.get('object')`, read at `7b64a3d73` with `tsx`), plus a grep of `packages/spec/src` for record- or array-valued declarations of the same property names. `deepRewrite` reads these properties by NAME, wherever they occur in any item. | property (site index, target `object`) | declared on | value shape | can collide? | measured at base | |:--|:--|:--|:--|:--| | `objects` | `permission` | record keyed by object name | yes | a grant dropped, `failed: []` (H1) | | `objects` | `translation` (`TranslationItemSchema`) | record keyed by object name | yes | one object's labels dropped: `{ dupsrc_customer: { label: 'KEHU-A' }, dupdst_customer: { label: 'KEHU-B' } }` copied as `{ dupdst_customer: { label: 'KEHU-B' } }`, `failed: []` | | `object` | `hook` (`string` or `string[]`) | array entries | yes | the copied object named twice, the handler ran twice (H4) | | `object` | `api`, `dashboard`, `dataset`, `field`, `object`, `page`, `permission`, `seed`, `view` | string | no: one string cannot meet another | not measured | | `objectName` | `action`, `app`, `object`, `tool`, and flow node configs | string | no | not measured | | `reference` | `object` (lookup fields) | string | no | not measured | | `targetObject` | `mapping` | string | no | not measured | The other record- or array-valued `objects` declarations in `packages/spec/src` (`websocket`, `manifest`, `app-install`, `deploy-bundle`, `stack`, `solution-blueprint`, the API's effective-permission answer) are not stored metadata item types, so `duplicatePackage` never copies them. The fix covers any record or array at any of these five properties, wherever it occurs. Three are pinned: `permission.objects` (both orders, with its negative control), `translation.objects` and `hook.object` (with its negative control). ## Pins New file: `packages/objectql/src/protocol-duplicate-package-objects-key-collision.test.ts`, beside `protocol-duplicate-package-permission-field-keys.test.ts`. It reads the stored rows out of the driver, never a payload. | case | at base `7b64a3d73` (probe) | record-key detection ablated | at head | |:--|:--|:--|:--| | a permission set whose renamed `objects` key meets one it holds is refused (the renamed key first). Located refusal, nothing written anywhere, source untouched. NEGATIVE CONTROL: the object still copied, and the write scan finds it | merged, `failed: []` | red | green | | the same, with the key it meets first | merged, `failed: []` | red | green | | NEGATIVE CONTROL: the same set without the second key copies, and its one grant follows the renamed object (`{ dupdst_customer: { allowRead: true } }`) | green (copies as today) | green | green | | a translation whose renamed `objects` key meets one it holds is refused, nothing written. NEGATIVE CONTROL: the object still copied, source untouched | merged, `failed: []` | red | green | | a hook whose renamed `object` entry meets one it holds is refused, nothing written. NEGATIVE CONTROL: the same hook without the second entry copies, its entry following the renamed object | duplicated entry, `failed: []` | green (the array arm is not ablated) | green | ## Ablation (one, on the collision detection) - **The mutation.** The record-key detection, `if (other !== undefined) {`, became `if (other !== undefined && other === 'ABLATED_23537') {`, so no record-key meeting is recorded. It ran through `scripts/ablation-replace.mjs` in wrap mode: anchor x1 to x0, blob `73a5c5e26` to `cca8c00e4`. - **The ablated run.** After a `metadata-protocol` rebuild, `ablation-dist-preflight` found the marker in 2 dist files (`index.js`, `index.cjs`). The pin went 3 failed / 2 passed. Each of the 3 failed for the base reason, `expected [] to have a length of 1 but got +0`: the two permission-set cases and the translation case. The hook case and the negative control stayed green, so the ablation is specific to the record-key arm. - **The restore.** The tool proved it: the blob equals HEAD `73a5c5e26`, and `git diff HEAD` is empty. The whole-tree `git status --porcelain` was empty. After a rebuild, `ablation-dist-preflight --absent` found the marker in none of 24 dist files and the tree clean against HEAD. The three duplicate-package pin files then went 14 passed (5 new, 5 in `…-permission-field-keys`, 4 in `…-unprefixed-object`). ## Tests and gates (at `1b8e8e13b`) - `pnpm --filter @objectstack/metadata-protocol exec vitest run --maxWorkers=2`: 236 files passed, 3 skipped; 28409 tests passed, 19 skipped (lock VERDICT command-exit 0). - `pnpm --filter @objectstack/objectql exec vitest run --project local --maxWorkers=2`: 419 files and 8113 tests passed, the new file included (lock VERDICT command-exit 0). - `metadata-protocol` typecheck: exit 0. - `objectql` typecheck: exit 0, and `check:test-typecheck` is OK with its debt unchanged (40 files / 234 errors / 65 signatures). The new file is in the `tsconfig.test.json` program (`--listFilesOnly`: 1 hit). - `eslint --no-inline-config` on the two changed source files: exit 0, no output. - `node scripts/pm/dispatch-gates.mjs --commands` derived 67 families at `1b8e8e13b`, the same 67 commands as the PM's derivation at `7b64a3d73`. All were run locally, and `--ran` with recorded exit codes reconciles: 67 derived, 66 run, 1 NOT-MEASURED, 0 UNRUN. - `check:lean-entry-closure` first refused its prerequisite (exit 3, `packages/objectql/dist/core.*` absent). It reran with exit 0 after an `objectql` build. - `check:dual-build-cjs-loads` is NOT MEASURED: `PREREQUISITE NOT MET` (exit 3), because it needs the whole workspace built. It is left to CI. - Not run locally: the downstream consumers of `@objectstack/metadata-protocol`. The declaration files changed only in this method's docblock, and the behaviour change is the refusal itself, which no other package seeds. CI runs them. - `origin/main` has moved two commits since the base (`b9b618942`, cli; `0fb55fc2f`, mcp), both disjoint from this diff. `git merge-tree --write-tree` is clean. No merge was made; the PR's merge-ref CI covers the joint state. ## Acceptance notes - **H4 is a judgement call, stated above.** The array arm refuses a hook whose renamed entry meets one it holds. If the reviewer reads the dispatch's "drops or widens" condition as not met by a doubled handler run, the arm is one hunk of `deepRewrite` and its pin is one case. - **An entry the source itself repeats is copied as stored** (`['dupsrc_customer', 'dupsrc_customer']` becomes `['dupdst_customer', 'dupdst_customer']`). The copy does exactly what the source does. Read from the code, not pinned. - **Read only, not measured:** the five string-valued sites in the table cannot collide by shape. ## Out of scope Nothing new found. #23538's two defects (the field `name` half-rename and the view-item name) are untouched. --- _Generated by [Claude Code](https://claude.ai/code/session_01YRp5z4kK6vgYkrdH97ztsy)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 0fb55fc commit 8226755

3 files changed

Lines changed: 335 additions & 4 deletions

File tree

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
---
2+
'@objectstack/metadata-protocol': patch
3+
---
4+
5+
fix(metadata-protocol): `duplicatePackage` refuses a copied item whose renamed object reference meets a name the item already holds, instead of merging the two
6+
7+
Clause-②: no
8+
9+
`duplicatePackage` (ADR-0070 D4, the package duplicate route's method) renames every copied object into the target namespace and rewrites each reference to it where a property names an object: a string, an array entry, or a record key (a permission set's `objects`, a translation's `objects`).
10+
11+
**FROM.** When a copied item also named the object the copy is about to be called, the renamed name landed on a name the item already held, and the copy merged the two with `failed: []`. A permission set holding `objects: { crm_customer: { allowRead: true }, crm2_customer: { allowRead: true, allowEdit: true, allowDelete: true } }`, duplicated into namespace `crm2`, was copied as `objects: { crm2_customer: { allowRead: true, allowEdit: true, allowDelete: true } }`: whichever entry came last was kept, so one grant was dropped, and here the copied object gained edit and delete the source set never granted on its own object. A translation's `objects` dropped one object's labels the same way. A hook whose `object` list held both names was copied as `['crm2_customer', 'crm2_customer']`, so its handler ran twice per event on the copied object.
12+
13+
**TO.** Such an item is listed in `failed[]`, and nothing of it is written (no item row, no history entry). The entry names the item, the property, the name that moved and the name it met, and the source edit that clears it, for example: "Permission set 'crm_access' is not copied: its `objects` key 'crm_customer' follows the copied object to 'crm2_customer', a key it already holds there, which this copy does not rename. … Remove 'crm_customer' or 'crm2_customer' on the source, then duplicate again." Remove one of the two names on the source item, then duplicate again.
14+
15+
**Unchanged.** An item with no such collision copies as before, its references following the copied objects. An entry the source item itself repeats is copied as stored. The source package is never modified.

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

Lines changed: 72 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26327,6 +26327,11 @@ export class ObjectStackProtocolImplementation implements
2632726327
* A copied permission set's field rules follow too: in `fields`, keyed
2632826328
* `<object>.<field>`, a key's object part takes the renamed object's name
2632926329
* and its field part is left as stored.
26330+
*
26331+
* A rename at a reference site that meets a name the item already holds
26332+
* there (a permission set's or a translation's `objects` key, a hook's
26333+
* `object` entry) is never merged: the item is refused in `failed[]`,
26334+
* naming both, before anything is written for it.
2633026335
*/
2633126336
async duplicatePackage(request: {
2633226337
sourcePackageId: string;
@@ -26550,26 +26555,75 @@ export class ObjectStackProtocolImplementation implements
2655026555
// KEY (`permission.objects` is keyed by object name) or a `.name`. The
2655126556
// record key holds for prefixed names too: a copied permission set
2655226557
// otherwise kept `objects.<source object>`, a grant on the SOURCE.
26558+
//
26559+
// ⛔ REFUSED, NEVER MERGED: at a reference site, a renamed name can
26560+
// meet a name the item already holds there — the source also names
26561+
// the object the copy is about to be called (`dupdst_customer`, an
26562+
// object outside the package or not yet created) beside its own
26563+
// (`dupsrc_customer`). Two record KEYS that land on one key keep only
26564+
// the last entry: a permission set's `objects` dropped a grant (and
26565+
// the copied object could gain edit and delete its source never gave
26566+
// it), a translation's `objects` dropped a label. Two array ELEMENTS
26567+
// that land on one name bind it twice: a hook's handler ran twice per
26568+
// event on the copied object. So every such meeting is recorded in
26569+
// `collisions` — the path, the name that moved, the name it met — and
26570+
// the call site refuses the item in `failed[]` before anything is
26571+
// written for it. `renameMap` is one-to-one, so a meeting is always a
26572+
// renamed name and a name this copy does not rename; an element the
26573+
// source itself repeats is copied as stored.
2655326574
const objectReferenceProperties = new Set(
2655426575
(REFERENCE_SITES.byTarget.get('object') ?? []).map((site) => site.property),
2655526576
);
26556-
const deepRewrite = (v: any, atReference = false): any => {
26577+
type ReferenceCollision = { at: string; shape: 'key' | 'entry'; moved: string; met: string };
26578+
const deepRewrite = (v: any, collisions: ReferenceCollision[], atReference = false, at = ''): any => {
2655726579
if (typeof v === 'string') {
2655826580
const whole = atReference ? renameMap.get(v) : undefined;
2655926581
if (whole !== undefined) return whole;
2656026582
return re ? v.replace(re, (m) => renameMap.get(m) ?? m) : v;
2656126583
}
26562-
if (Array.isArray(v)) return v.map((element) => deepRewrite(element, atReference));
26584+
if (Array.isArray(v)) {
26585+
const out = v.map((element, i) => deepRewrite(element, collisions, atReference, `${at}[${i}]`));
26586+
if (atReference) {
26587+
const firstAt = new Map<string, number>();
26588+
out.forEach((element, i) => {
26589+
if (typeof element !== 'string') return;
26590+
const j = firstAt.get(element);
26591+
if (j === undefined) {
26592+
firstAt.set(element, i);
26593+
} else if (v[j] !== v[i]) {
26594+
collisions.push({ at, shape: 'entry', moved: v[i] !== element ? v[i] : v[j], met: element });
26595+
}
26596+
});
26597+
}
26598+
return out;
26599+
}
2656326600
if (v && typeof v === 'object') {
2656426601
const o: any = {};
26602+
const from = new Map<string, string>();
2656526603
for (const [k, val] of Object.entries(v)) {
2656626604
const key = atReference ? (renameMap.get(k) ?? k) : k;
26567-
o[key] = deepRewrite(val, objectReferenceProperties.has(k) || (atReference && k === 'name'));
26605+
const other = from.get(key);
26606+
if (other !== undefined) {
26607+
collisions.push({ at, shape: 'key', moved: key === k ? other : k, met: key });
26608+
}
26609+
from.set(key, k);
26610+
o[key] = deepRewrite(
26611+
val, collisions,
26612+
objectReferenceProperties.has(k) || (atReference && k === 'name'),
26613+
at ? `${at}.${k}` : k,
26614+
);
2656826615
}
2656926616
return o;
2657026617
}
2657126618
return v;
2657226619
};
26620+
const describeCollision = (c: ReferenceCollision): string =>
26621+
`its \`${c.at}\` ${c.shape} '${c.moved}' follows the copied object to '${c.met}', `
26622+
+ `${c.shape === 'key' ? 'a key' : 'an entry'} it already holds there, which this copy does not rename. `
26623+
+ (c.shape === 'key'
26624+
? 'A record keeps one entry per key, so the copy would drop one of the two. '
26625+
: `The copy would name '${c.met}' twice. `)
26626+
+ `Remove '${c.moved}' or '${c.met}' on the source, then duplicate again.`;
2657326627
// ⭐ A COPIED PERMISSION SET'S FIELD RULES FOLLOW THE RENAMED OBJECTS.
2657426628
// `PermissionSetSchema.fields` is a record keyed `<entity_name>.<field_name>`
2657526629
// (`packages/spec/src/security/permission.zod.ts`), and the security
@@ -26738,7 +26792,21 @@ export class ObjectStackProtocolImplementation implements
2673826792
continue;
2673926793
}
2674026794
}
26741-
const rewritten = deepRewrite(item);
26795+
const collisions: ReferenceCollision[] = [];
26796+
const rewritten = deepRewrite(item, collisions);
26797+
if (collisions.length > 0) {
26798+
const noun = singular === 'permission'
26799+
? 'Permission set'
26800+
: `${singular.charAt(0).toUpperCase()}${singular.slice(1)}`;
26801+
const more = collisions.length - 1;
26802+
failed.push({
26803+
type: row.type,
26804+
name: row.name,
26805+
error: `${noun} '${row.name}' is not copied: ${describeCollision(collisions[0]!)}`
26806+
+ (more > 0 ? ` It has ${more} more such collision${more === 1 ? '' : 's'}.` : ''),
26807+
});
26808+
continue;
26809+
}
2674226810
if (rewritten && typeof rewritten === 'object' && !Array.isArray(rewritten)) {
2674326811
rewritten.name = newName;
2674426812
// [ADR-0086 D3] A permission set that declares its SOURCE

0 commit comments

Comments
 (0)