Skip to content

Commit 71af9f5

Browse files
os-zhuangclaude
andauthored
test(cli),fix(sharing): migrate-meta end-to-end proof + inert-rule warn dedup (#4066)
* test(cli): os migrate meta gets its end-to-end proof — the real CLI over a pre-17 project The chain's transforms were fixture-tested in spec, but the COMMAND path — bundleRequire config loading, the convert:false normalization that keeps rewrites attributable to chain hops, the schema-validity verdict, the machine JSON shape, --out snapshots, the support-floor refusal — had no test. Spawns bin/run-dev.js against a temp project authored in the pre-17 dialect (one key from every v17 conversion family in a single pass) and pins: - all 11 v17 conversion families attribute at least one rewrite; - semantic TODOs surface with reason + acceptanceCriteria, never auto-apply; - the --out snapshot re-parses under the CURRENT schema, with per-family spot-checks (execute→target, storage.notNull stamped on the required field and NOT on the optional one, full→edit, every close-out key gone, and the canonical protocol-11 filter key passing through untouched); - CHAIN IDEMPOTENCE end-to-end: replaying --from 16 on the migrated snapshot applies zero changes and stays schema-valid; - --from 9 exits non-zero with the structured unsupported_from_major error. Fixture authoring found one doc-comment error in my own draft: the filters→filter alias is protocol-11 dialect, not 16 — a 16-authored source already says filter, and the test now pins the pass-through instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QuViRSR1j6GJjf9qGbnqFX * fix(sharing): inert-rule warn dedup + boot aggregate (#3929 follow-up) The fail-closed evaluator warned on EVERY pass (findMatchingRecords per evaluation AND recordMatches per reconciled write) — one legacy criteria-less rule could dominate a deployment's log. Now: once per rule per process (instance Set, message says so and names the fix), plus one aggregate line from the boot backfill (count + rule names). Enforcement unchanged on every pass — the dedup test pins zero grants throughout and that the backfill walk does not re-warn per rule. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QuViRSR1j6GJjf9qGbnqFX --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent fae74b5 commit 71af9f5

5 files changed

Lines changed: 330 additions & 4 deletions

File tree

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
---
2+
"@objectstack/plugin-sharing": patch
3+
---
4+
5+
fix(sharing): the criteria-less-rule warn is once per rule per process, plus one boot aggregate (#3929 follow-up)
6+
7+
Pre-dedup the fail-closed evaluator warned on EVERY pass — per evaluation and
8+
per reconciled write — so one legacy criteria-less rule could dominate a
9+
deployment's log. Enforcement is unchanged (such a rule still matches
10+
nothing and its grants are revoked on reconcile); the warn now fires once
11+
per rule per process, and the boot backfill emits a single operator-facing
12+
aggregate (count + rule names + the fix: repair the criteria or set
13+
active: false).
Lines changed: 245 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,245 @@
1+
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* `os migrate meta` — end-to-end over the REAL CLI process (ADR-0087 D3/D5).
5+
*
6+
* The chain's transforms are fixture-tested in @objectstack/spec
7+
* (conversions.test.ts, migrations.test.ts), but the COMMAND path — config
8+
* loading via bundleRequire, the convert:false normalization that keeps the
9+
* chain attributable, the schema-validity verdict, the machine JSON shape,
10+
* `--out` snapshots, and the support-floor refusal — had no test at all.
11+
* This spawns `bin/run-dev.js` against a temp project authored in the
12+
* PRE-protocol-17 dialect, exercising one key from every v17 conversion
13+
* family in a single pass, exactly the cross-major "consumer upgrades"
14+
* scenario the command exists for.
15+
*/
16+
17+
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
18+
import { execFile } from 'node:child_process';
19+
import { promisify } from 'node:util';
20+
import { mkdtempSync, rmSync, writeFileSync, readFileSync } from 'node:fs';
21+
import { tmpdir } from 'node:os';
22+
import { join, resolve } from 'node:path';
23+
import { fileURLToPath } from 'node:url';
24+
import { ObjectStackDefinitionSchema } from '@objectstack/spec';
25+
26+
const execFileP = promisify(execFile);
27+
const HERE = resolve(fileURLToPath(import.meta.url), '..');
28+
const CLI = resolve(HERE, '../bin/run-dev.js');
29+
const TSX = resolve(HERE, '../../../node_modules/.bin/tsx');
30+
31+
/**
32+
* A stack authored against protocol 16: every line marked `// 16:` is a shape
33+
* the v17 chain must rewrite, spanning each conversion family — renames
34+
* (action execute→target, sharing full→edit), the required→storage.notNull
35+
* explicitization, and the #3896 close-out removals (rls.priority, the four
36+
* tool keys, flow active/template/outputSchema/fallbackNodeId, view/dashboard
37+
* inert keys, agent.knowledge, skill.triggerPhrases).
38+
*/
39+
const PRE17_CONFIG = `
40+
export default {
41+
name: 'migrate_meta_e2e',
42+
label: 'Migrate Meta E2E',
43+
objects: [{
44+
name: 'e2e_ticket',
45+
label: 'Ticket',
46+
fields: {
47+
title: { type: 'text', label: 'Title', required: true }, // 16: required implied NOT NULL
48+
notes: { type: 'textarea', label: 'Notes' },
49+
},
50+
}],
51+
actions: [{
52+
name: 'close_ticket',
53+
label: 'Close',
54+
type: 'script',
55+
execute: 'closeHandler', // 16: renamed to target
56+
shortcut: 'Ctrl+K', // 16: removed (never dispatched)
57+
bulkEnabled: true, // 16: removed (toolbar reads view bulkActions)
58+
}],
59+
flows: [{
60+
name: 'e2e_flow',
61+
label: 'E2E Flow',
62+
type: 'autolaunched',
63+
active: false, // 16: removed (status is the lifecycle)
64+
template: true, // 16: removed (no reader)
65+
errorHandling: { strategy: 'retry', fallbackNodeId: 'n9' }, // 16: fault edges own this
66+
nodes: [
67+
{ id: 'n1', type: 'start', label: 'Start', outputSchema: { ok: { type: 'boolean' } } }, // 16: removed
68+
{ id: 'n2', type: 'delete_record', label: 'Purge', config: { objectName: 'e2e_ticket', filter: { done: true } } }, // canonical since protocol 11 — must pass through untouched
69+
],
70+
edges: [],
71+
}],
72+
views: [{
73+
object: 'e2e_ticket',
74+
list: { type: 'grid', columns: ['title'], responsive: { hiddenOn: ['xs'] }, performance: { lazyLoad: true } }, // 16: removed
75+
form: { type: 'simple', sections: [{ fields: ['title'] }], defaultSort: [{ field: 'title' }], aria: { label: 'Ticket' } }, // 16: removed
76+
}],
77+
dashboards: [{
78+
name: 'e2e_kpis',
79+
label: 'E2E KPIs',
80+
aria: { label: 'KPIs' }, // 16: removed
81+
performance: { prefetch: true },// 16: removed
82+
widgets: [],
83+
}],
84+
agents: [{
85+
name: 'e2e_agent',
86+
label: 'Agent',
87+
role: 'Helper',
88+
instructions: 'help',
89+
knowledge: { sources: ['faq'] }, // 16: removed (never scoped retrieval)
90+
}],
91+
skills: [{
92+
name: 'e2e_skill',
93+
label: 'Skill',
94+
tools: ['query_records'],
95+
triggerPhrases: ['do the thing'], // 16: removed (never matched)
96+
}],
97+
tools: [{
98+
name: 'e2e_tool',
99+
label: 'Tool',
100+
description: 'A tool',
101+
parameters: { type: 'object' },
102+
category: 'action', // 16: removed
103+
permissions: ['x.y'], // 16: removed (gated nothing)
104+
active: true, // 16: removed (withdrew nothing)
105+
builtIn: false, // 16: removed
106+
}],
107+
permissions: [{
108+
name: 'e2e_set',
109+
label: 'Set',
110+
objects: { e2e_ticket: { allowRead: true } },
111+
rowLevelSecurity: [{
112+
name: 'own_rows',
113+
object: 'e2e_ticket',
114+
operation: 'select',
115+
using: 'owner = current_user.id',
116+
enabled: true,
117+
priority: 10, // 16: removed (no conflict to order)
118+
}],
119+
}],
120+
sharingRules: [{
121+
name: 'share_hot',
122+
type: 'criteria',
123+
object: 'e2e_ticket',
124+
label: 'Hot tickets',
125+
condition: "record.hot == true",
126+
sharedWith: { type: 'team', value: 'support' },
127+
accessLevel: 'full', // 16: full→edit (walked at the TOP-LEVEL collection)
128+
}],
129+
};
130+
`;
131+
132+
/** Conversion ids the run MUST attribute at least one rewrite to. */
133+
const EXPECTED_CONVERSIONS = [
134+
'action-execute-to-target',
135+
'action-inert-keys-removed',
136+
'flow-inert-keys-removed',
137+
'view-inert-keys-removed',
138+
'dashboard-inert-keys-removed',
139+
'agent-knowledge-removed',
140+
'skill-trigger-phrases-removed',
141+
'tool-inert-authoring-keys-removed',
142+
'permission-rls-priority-removed',
143+
'field-required-notnull-explicit',
144+
'sharing-rule-access-level-full-to-edit',
145+
];
146+
147+
let dir: string;
148+
let out: { stdout: string; parsed: any };
149+
150+
async function runMeta(args: string[], cwd: string) {
151+
const { stdout } = await execFileP(TSX, [CLI, 'migrate', 'meta', ...args], {
152+
cwd,
153+
maxBuffer: 16 * 1024 * 1024,
154+
env: { ...process.env, NO_COLOR: '1' },
155+
});
156+
return stdout;
157+
}
158+
159+
beforeAll(async () => {
160+
dir = mkdtempSync(join(tmpdir(), 'os-migrate-meta-e2e-'));
161+
writeFileSync(join(dir, 'objectstack.config.ts'), PRE17_CONFIG);
162+
const stdout = await runMeta(['--from', '16', '--json', '--out', join(dir, 'migrated.json')], dir);
163+
out = { stdout, parsed: JSON.parse(stdout) };
164+
}, 120_000);
165+
166+
afterAll(() => {
167+
try { rmSync(dir, { recursive: true, force: true }); } catch { /* ignore */ }
168+
});
169+
170+
describe('os migrate meta --from 16 (e2e over the real CLI)', () => {
171+
it('replays the v17 chain and reports schema-valid output', () => {
172+
expect(out.parsed.from).toBe(16);
173+
expect(out.parsed.schemaValid).toBe(true);
174+
});
175+
176+
it('attributes at least one rewrite to every expected v17 conversion family', () => {
177+
const ids = new Set(out.parsed.applied.map((a: any) => a.conversionId));
178+
for (const id of EXPECTED_CONVERSIONS) {
179+
expect(ids.has(id), `expected a rewrite from ${id}; got ${[...ids].join(', ')}`).toBe(true);
180+
}
181+
});
182+
183+
it('surfaces the semantic TODOs instead of auto-applying them', () => {
184+
expect(Array.isArray(out.parsed.todos)).toBe(true);
185+
expect(out.parsed.todos.length).toBeGreaterThan(0);
186+
for (const t of out.parsed.todos) {
187+
expect(t.reason?.length).toBeGreaterThan(0);
188+
expect(t.acceptanceCriteria?.length).toBeGreaterThan(0);
189+
}
190+
});
191+
192+
it('the --out snapshot re-parses under the CURRENT schema and carries the rewrites', () => {
193+
const snap = JSON.parse(readFileSync(join(dir, 'migrated.json'), 'utf-8'));
194+
const parsed = ObjectStackDefinitionSchema.safeParse(snap);
195+
expect(parsed.success, JSON.stringify(parsed.success ? '' : parsed.error.issues.slice(0, 3))).toBe(true);
196+
197+
// Spot-check one rewrite per family on the snapshot itself.
198+
expect(snap.actions[0].target).toBe('closeHandler');
199+
expect(snap.actions[0].execute).toBeUndefined();
200+
expect(snap.actions[0].shortcut).toBeUndefined();
201+
expect(snap.flows[0].active).toBeUndefined();
202+
expect(snap.flows[0].errorHandling.fallbackNodeId).toBeUndefined();
203+
expect(snap.flows[0].nodes[0].outputSchema).toBeUndefined();
204+
expect(snap.flows[0].nodes[1].config.filter, 'canonical key passes through untouched').toEqual({ done: true });
205+
expect(snap.views[0].list.responsive).toBeUndefined();
206+
expect(snap.views[0].form.defaultSort).toBeUndefined();
207+
expect(snap.dashboards[0].aria).toBeUndefined();
208+
expect(snap.agents[0].knowledge).toBeUndefined();
209+
expect(snap.skills[0].triggerPhrases).toBeUndefined();
210+
expect(snap.tools[0].category).toBeUndefined();
211+
expect(snap.permissions[0].rowLevelSecurity[0].priority).toBeUndefined();
212+
expect(snap.sharingRules[0].accessLevel).toBe('edit');
213+
// ADR-0113 explicitization: the pre-17 required field carries its column
214+
// constraint in writing; the optional field gains nothing.
215+
expect(snap.objects[0].fields.title.storage).toEqual({ notNull: true });
216+
expect(snap.objects[0].fields.notes.storage).toBeUndefined();
217+
});
218+
219+
it('is idempotent: replaying the chain on the migrated snapshot applies zero changes', async () => {
220+
const dir2 = mkdtempSync(join(tmpdir(), 'os-migrate-meta-e2e2-'));
221+
try {
222+
const snap = readFileSync(join(dir, 'migrated.json'), 'utf-8');
223+
writeFileSync(join(dir2, 'objectstack.config.ts'), `export default ${snap};`);
224+
const stdout = await runMeta(['--from', '16', '--json'], dir2);
225+
const again = JSON.parse(stdout);
226+
expect(again.applied).toEqual([]);
227+
expect(again.schemaValid).toBe(true);
228+
} finally {
229+
try { rmSync(dir2, { recursive: true, force: true }); } catch { /* ignore */ }
230+
}
231+
}, 120_000);
232+
233+
it('refuses a --from below the support floor with the structured error', async () => {
234+
let failed = false;
235+
try {
236+
await runMeta(['--from', '9', '--json'], dir);
237+
} catch (e: any) {
238+
failed = true;
239+
const body = JSON.parse(String(e.stdout || '{}'));
240+
expect(body.error).toBe('unsupported_from_major');
241+
expect(body.message).toMatch(/floor|support/i);
242+
}
243+
expect(failed, 'expected a non-zero exit below the floor').toBe(true);
244+
}, 120_000);
245+
});

packages/plugins/plugin-sharing/src/boot-backfill.test.ts

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,3 +206,41 @@ describe("backfillRetiredAccessLevels (#3865 — stored 'full' normalises to 'ed
206206
expect(engine._tables.sys_record_share[0].access_level).toBe('full');
207207
});
208208
});
209+
210+
describe('#3929 follow-up — inert-rule warn dedup + boot aggregate', () => {
211+
it('warns ONCE per inert rule per process, and the boot pass emits one aggregate', async () => {
212+
const engine = makeEngine();
213+
const sharing = new SharingService({ engine: engine as any });
214+
const warn = vi.fn();
215+
const rules = new SharingRuleService({ engine: engine as any, sharing, logger: { warn } as any });
216+
217+
engine._tables.sys_sharing_rule = [
218+
{ id: 'r_legacy', name: 'legacy_match_all', object_name: 'showcase_inquiry', criteria: null, recipient_type: 'position', recipient_id: 'p1', access_level: 'read', active: true },
219+
{ id: 'r_legacy2', name: 'legacy_match_all_2', object_name: 'showcase_inquiry', criteria: {}, recipient_type: 'position', recipient_id: 'p1', access_level: 'read', active: true },
220+
];
221+
engine._tables.showcase_inquiry = [{ id: 'inq_1', status: 'new' }];
222+
223+
// Repeated evaluations — the pre-dedup behavior warned on every one.
224+
await rules.evaluateRule('r_legacy', SYS);
225+
await rules.evaluateRule('r_legacy', SYS);
226+
await rules.evaluateRule('r_legacy2', SYS);
227+
await rules.evaluateRule('r_legacy', SYS);
228+
229+
const perRule = warn.mock.calls.filter(([msg]) => String(msg).includes('no usable criteria'));
230+
expect(perRule).toHaveLength(2); // once per DISTINCT rule, not per pass
231+
expect(perRule.map(([, meta]) => (meta as any).rule).sort()).toEqual(['legacy_match_all', 'legacy_match_all_2']);
232+
233+
// Enforcement unchanged on every pass: nothing granted, ever.
234+
expect(engine._tables.sys_record_share ?? []).toHaveLength(0);
235+
236+
// The boot backfill walks the same rules and adds ONE aggregate line.
237+
const bootWarn = vi.fn();
238+
await backfillRuleGrants(rules, engine._tables.sys_sharing_rule, { warn: bootWarn });
239+
const agg = bootWarn.mock.calls.filter(([msg]) => String(msg).includes('matching NO records'));
240+
expect(agg).toHaveLength(1);
241+
expect((agg[0][1] as any).count).toBe(2);
242+
expect((agg[0][1] as any).rules.sort()).toEqual(['r_legacy', 'r_legacy2']);
243+
// …and the per-rule warns did NOT repeat during the backfill pass.
244+
expect(warn.mock.calls.filter(([msg]) => String(msg).includes('no usable criteria'))).toHaveLength(2);
245+
});
246+
});

packages/plugins/plugin-sharing/src/sharing-plugin.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,17 @@ export async function backfillRuleGrants(
7272
ms: Date.now() - start,
7373
});
7474
}
75+
// [#3929 follow-up] One aggregate line for the legacy criteria-less rules
76+
// the pass just walked (each also warned once, above, via the per-rule
77+
// dedup): the operator-facing summary of what is under-sharing and why.
78+
const inert = ruleService.inertRuleNames;
79+
if (inert.length > 0) {
80+
logger?.warn?.(
81+
'SharingServicePlugin: rule(s) with no usable criteria are matching NO records — their ' +
82+
'grants are revoked on reconcile (ADR-0049). Fix the criteria or set active: false.',
83+
{ count: inert.length, rules: inert },
84+
);
85+
}
7586
return reconciled;
7687
}
7788

packages/plugins/plugin-sharing/src/sharing-rule-service.ts

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,15 @@ export class SharingRuleService implements ISharingRuleService {
6767
private readonly engine: SharingEngine;
6868
private readonly sharing: SharingService;
6969
private readonly logger?: SharingRuleServiceOptions['logger'];
70+
/**
71+
* [#3929 follow-up] Inert (criteria-less) rules seen this process, for
72+
* once-per-rule warn dedup + the boot aggregate. Pre-dedup the evaluator
73+
* warned on EVERY pass — findMatchingRecords per evaluation AND
74+
* recordMatches per reconciled write — so one legacy row could dominate a
75+
* deployment's log. The enforcement is unchanged (such a rule still
76+
* matches NOTHING); only the repetition is gone.
77+
*/
78+
private readonly inertRuleSeen = new Set<string>();
7079

7180
constructor(opts: SharingRuleServiceOptions) {
7281
this.engine = opts.engine;
@@ -301,13 +310,23 @@ export class SharingRuleService implements ISharingRuleService {
301310
*/
302311
private isInertMatchAll(rule: SharingRuleRow): boolean {
303312
if (!isMatchAllCriteria(rule.criteria)) return false;
304-
this.logger?.warn?.(
305-
'[sharing-rule] rule has no usable criteria — matching NO records instead of every record (ADR-0049)',
306-
{ rule: rule.name, object: rule.object_name },
307-
);
313+
const key = String(rule.id ?? rule.name);
314+
if (!this.inertRuleSeen.has(key)) {
315+
this.inertRuleSeen.add(key);
316+
this.logger?.warn?.(
317+
'[sharing-rule] rule has no usable criteria — matching NO records instead of every record ' +
318+
'(ADR-0049; logged once per rule per process — fix the criteria or set active: false)',
319+
{ rule: rule.name, object: rule.object_name },
320+
);
321+
}
308322
return true;
309323
}
310324

325+
/** Names of inert (criteria-less) rules seen so far — the boot aggregate reads this. */
326+
get inertRuleNames(): readonly string[] {
327+
return [...this.inertRuleSeen];
328+
}
329+
311330
private async findMatchingRecords(rule: SharingRuleRow): Promise<string[]> {
312331
if (this.isInertMatchAll(rule)) return [];
313332
const filter = (rule.criteria ?? {}) as any;

0 commit comments

Comments
 (0)