Skip to content

Commit 625a7cb

Browse files
committed
refactor(metadata-protocol): remove the unreachable legacy raw-engine save branch (#5264)
`saveMetaItem` had two persistence routes: the repository write path (history row + watch event + monotonic `seq`) and a legacy raw-engine branch after it (`engine.insert`/`engine.update` straight into `sys_metadata` — no history, no watch event, no `seq`). The legacy branch ran when `isOverlayAllowed(type) || isRuntimeCreateAllowed(type)` was false. #5086 (PR #5263) made that condition unreachable here: the code-only refusal earlier in the same method throws on exactly that predicate, on every kernel (no longer keyed on `environmentId`), over the same canonicalized type key. `OS_METADATA_WRITABLE` is not a hole either — unlocking a type there makes `isOverlayAllowed` true and routes the save back through the repository. No behaviour change. `deleteMetaItem`'s structurally symmetric branch is untouched: it is still reachable and still necessary (a control-plane delete of a code-only row that predates the refusal is the repair action #5263 deliberately left open). It gained a comment saying why it survives. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01V7WetGmnfoXNn8cLieKKmx
1 parent 889ae47 commit 625a7cb

4 files changed

Lines changed: 351 additions & 255 deletions

File tree

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
---
2+
"@objectstack/metadata-protocol": patch
3+
---
4+
5+
refactor(metadata-protocol): 删除 `saveMetaItem` 里已不可达的 legacy raw-engine 写入分支 (#5264)
6+
7+
`saveMetaItem` 过去有两条持久化路径:repository 写入路径(追加
8+
`sys_metadata_history`、发 watch 事件、带单调 `seq`),以及其后的 legacy
9+
raw-engine 分支(直接 `engine.insert` / `engine.update``sys_metadata`,
10+
没有 history 行、没有 watch 事件、没有 `seq`,回执形如
11+
`Saved customization overlay (env-wide) — type=…`)。后者的进入条件是
12+
`isOverlayAllowed(type) || isRuntimeCreateAllowed(type)` 为假。
13+
14+
**没有行为变化 —— 这条分支在运行时已经到不了。** #5086(PR #5263)把
15+
code-only 类型的拒绝提到了同一方法更早的位置,并且不再以 `environmentId`
16+
为条件:它抛错的判据与上面那个条件恰好互为反面,读的还是同一个规范化后的
17+
类型键(`canonicalizeMetaRequestType` 在方法开头折叠单复数,两个标志读取器
18+
内部又各自折叠一次)。`OS_METADATA_WRITABLE` 也不是缺口:在那里解锁一个
19+
类型会让 `isOverlayAllowed` 为真,从而走回 repository 路径。因此凡是能走到
20+
分叉点的写入,一律走 repository 路径。
21+
22+
保留 `useRepoPath` 的代价不是多几行代码,而是它是一份 grep 得到、读起来
23+
像活代码的样板:照它推理会得出「`sys_metadata` 存在一个不写 history 的
24+
合法写入口」——现在没有了。
25+
26+
`deleteMetaItem` 里结构对称的那条 legacy 分支**一行未动**:它在
27+
control-plane kernel(`environmentId === undefined`)上删除 code-only 遗留行
28+
时仍然可达且必要(#5263 特意没有收紧删除侧,因为删除是修复动作),该分支上
29+
新增了说明它为何还活着的注释。

packages/metadata-protocol/src/protocol.code-only-types.test.ts

Lines changed: 124 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,11 @@ function makeStubEngine(artifacts: Array<{ type: string; name: string }> = []) {
8585
const artifactKeys = new Set(artifacts.map((a) => `${a.type}|${a.name}`));
8686
const keyOf = (w: Record<string, unknown>) =>
8787
`${w.type}|${w.name}|${w.organization_id ?? '__env__'}|${w.state ?? 'active'}`;
88+
// #5264 — every write the engine is asked for, in order. `rows` alone
89+
// cannot tell the two persistence routes apart (both end at
90+
// `insert('sys_metadata', …)`); the tell is whether a
91+
// `sys_metadata_history` append came with it. See the #5264 block below.
92+
const writes: Array<{ op: 'insert' | 'update' | 'delete'; table: string }> = [];
8893
const engine: any = {
8994
async findOne(_t: string, opts: { where: Record<string, unknown> }) {
9095
for (const row of rows.values()) {
@@ -96,14 +101,15 @@ function makeStubEngine(artifacts: Array<{ type: string; name: string }> = []) {
96101
},
97102
async find() { return []; },
98103
async insert(_t: string, data: Record<string, unknown>) {
104+
writes.push({ op: 'insert', table: _t });
99105
if (_t !== 'sys_metadata') return { id: 'side_effect_skip' };
100106
nextId += 1;
101107
const row = { id: `r_${nextId}`, ...(data as any) } as Row;
102108
rows.set(keyOf(data), row);
103109
return { id: row.id };
104110
},
105-
async update() { return { id: null }; },
106-
async delete() { return { deleted: 0 }; },
111+
async update(_t: string) { writes.push({ op: 'update', table: _t }); return { id: null }; },
112+
async delete(_t: string) { writes.push({ op: 'delete', table: _t }); return { deleted: 0 }; },
107113
registry: {
108114
registerItem: () => {},
109115
registerObject: () => {},
@@ -115,7 +121,7 @@ function makeStubEngine(artifacts: Array<{ type: string; name: string }> = []) {
115121
artifactKeys.has(`${type}|${name}`) ? { name, _packageId: 'showcase' } : undefined,
116122
},
117123
};
118-
return { engine, rows };
124+
return { engine, rows, writes };
119125
}
120126

121127
/**
@@ -132,13 +138,13 @@ const KERNELS: Array<{ label: string; environmentId?: string }> = [
132138
];
133139

134140
function makeProtocol(environmentId?: string, artifacts?: Array<{ type: string; name: string }>) {
135-
const { engine, rows } = makeStubEngine(artifacts);
141+
const { engine, rows, writes } = makeStubEngine(artifacts);
136142
const protocol = new ObjectStackProtocolImplementation(
137143
engine,
138144
() => new Map(),
139145
environmentId,
140146
) as any;
141-
return { protocol, rows };
147+
return { protocol, rows, writes };
142148
}
143149

144150
const metaRows = (rows: Map<string, Row>) => Array.from(rows.values());
@@ -321,4 +327,117 @@ describe('code-only metadata types are refused on every kernel (#5086)', () => {
321327
});
322328
}
323329
});
330+
331+
// ── #5264 — one persistence route, and the proof it is the only one ───
332+
//
333+
// `saveMetaItem` used to end in a legacy raw-engine branch: `engine.insert`
334+
// / `engine.update` straight into `sys_metadata`, no `sys_metadata_history`
335+
// append, no watch event, no `seq`. It ran exactly when
336+
// `isOverlayAllowed(type) || isRuntimeCreateAllowed(type)` was false —
337+
// which is the predicate the #5086 gate above throws on, unconditionally,
338+
// over the same canonicalized type key. #5264 removed the branch.
339+
//
340+
// These pins are about the RECEIPT, not the verdict, because the receipt is
341+
// the only thing that told the two routes apart from outside: the legacy
342+
// one answered `200 {"success":true,"message":"Saved customization overlay
343+
// (env-wide) — type=…"}` with no `state=`, no `[seq=…]`, and no history
344+
// row. That is precisely the answer #5086 caught the showcase giving for a
345+
// `job`. They are green before the removal too — a branch nothing reaches
346+
// is what "dead" means — so they are not a regression test for the
347+
// deletion; they are the guard that stops a second historyless write path
348+
// from being introduced, and they fail loudly if the #5086 gate is ever
349+
// narrowed back to `environmentId !== undefined`.
350+
describe('#5264 — saveMetaItem persists through the repository or not at all', () => {
351+
for (const type of CODE_ONLY_TYPES) {
352+
it(`asks the engine for NOTHING when refusing ${type} on a control-plane kernel`, async () => {
353+
// The exact condition the deleted branch claimed for itself:
354+
// "control-plane bootstrap (environmentId === undefined) for
355+
// non-overlay-allowed types". Driven here on purpose — the
356+
// refusal lands first, so the write never becomes a write.
357+
const probe = PROBES[type]!;
358+
const { protocol, rows, writes } = makeProtocol(undefined);
359+
360+
const err = await protocol
361+
.saveMetaItem({ type, name: probe.name, item: probe.item })
362+
.then(() => null, (e: any) => e);
363+
364+
expect(err?.status).toBe(403);
365+
expect(metaRows(rows)).toEqual([]);
366+
// Stronger than "no row landed": no write was even attempted,
367+
// so there is nothing for a raw-engine path to have done.
368+
expect(writes).toEqual([]);
369+
});
370+
}
371+
372+
// One savable type per shape the two-tier model distinguishes.
373+
const ACCEPTED: Array<{ type: string; item: Record<string, unknown> }> = [
374+
{
375+
type: 'view', // allowOrgOverride + allowRuntimeCreate
376+
item: {
377+
name: 'rc3_receipt_view',
378+
label: 'Receipt',
379+
object: 'task',
380+
columns: [{ field: 'name', label: 'Name' }],
381+
},
382+
},
383+
{
384+
type: 'hook', // allowRuntimeCreate only
385+
item: { name: 'rc3_receipt_view', object: 'task', events: ['beforeUpdate'] },
386+
},
387+
{
388+
type: 'theme', // no static registry entry (plugin-registered)
389+
item: { name: 'rc3_receipt_view', label: 'Receipt', tokens: {} },
390+
},
391+
];
392+
393+
for (const { label, environmentId } of KERNELS) {
394+
for (const { type, item } of ACCEPTED) {
395+
it(`answers a ${type} save with a repository receipt on a ${label}`, async () => {
396+
const { protocol, writes } = makeProtocol(environmentId);
397+
398+
const result = await protocol.saveMetaItem({
399+
type,
400+
name: 'rc3_receipt_view',
401+
item,
402+
...(environmentId ? { organizationId: 'org_alpha' } : {}),
403+
});
404+
405+
expect(result.success).toBe(true);
406+
// `seq` and `state` exist only on the repository receipt.
407+
expect(typeof result.seq).toBe('number');
408+
expect(result.state).toBe('active');
409+
expect(result.message).toContain('[seq=');
410+
// And the change log really was appended — the legacy
411+
// branch's defining omission.
412+
expect(writes.some((w) => w.table === 'sys_metadata_history')).toBe(true);
413+
});
414+
}
415+
}
416+
417+
for (const type of CODE_ONLY_TYPES) {
418+
it(`routes ${type} back through the repository once OS_METADATA_WRITABLE unlocks it`, async () => {
419+
// The last leg of the unreachability argument: the escape hatch
420+
// does not open a second door. Unlocking a type makes
421+
// `isOverlayAllowed` true, which is the same predicate the
422+
// repository path is chosen by — so an unlocked save is a
423+
// repository save, receipt and history row included.
424+
const probe = PROBES[type]!;
425+
process.env.OS_METADATA_WRITABLE = type;
426+
ObjectStackProtocolImplementation.resetEnvWritableCache();
427+
resetEnvWritableMetadataTypes();
428+
429+
const { protocol, writes } = makeProtocol(undefined);
430+
const result = await protocol.saveMetaItem({
431+
type,
432+
name: probe.name,
433+
item: probe.item,
434+
});
435+
436+
expect(result.success).toBe(true);
437+
expect(typeof result.seq).toBe('number');
438+
expect(result.message).toContain('[seq=');
439+
expect(writes.some((w) => w.table === 'sys_metadata_history')).toBe(true);
440+
});
441+
}
442+
});
324443
});

packages/metadata-protocol/src/protocol.stored-migration.test.ts

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -463,9 +463,14 @@ describe('migrateStoredMetadata — what it declines to touch, loudly (#4327)',
463463
});
464464

465465
it('skips a type with no repository write path rather than rewriting it without history', async () => {
466-
// `agent` is allowOrgOverride:false + allowRuntimeCreate:false, so
467-
// `saveMetaItem` would take the legacy raw-engine branch: no history row
468-
// and a forced `state: 'active'`. Declining beats a silent half-write.
466+
// `agent` is allowOrgOverride:false + allowRuntimeCreate:false. The
467+
// skip is what this test pins, and it is unchanged; only its rationale
468+
// moved. It used to be "`saveMetaItem` would take the legacy raw-engine
469+
// branch: no history row and a forced `state: 'active'`" — #5086
470+
// (PR #5263) then made `saveMetaItem` refuse a code-only type with 403
471+
// before persistence, and #5264 deleted the now-unreachable branch. So
472+
// today the pass declines a write that would be refused anyway, which
473+
// keeps it a reported `skipped` row instead of `failed` noise.
469474
const { engine, tables } = makeStubEngine([
470475
{ type: 'agent', name: 'legacy_agent', metadata: { name: 'legacy_agent', label: 'Legacy' } },
471476
]);

0 commit comments

Comments
 (0)