From eb7972047627b07c72311cb43d66e2a20f99a764 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 07:25:11 +0000 Subject: [PATCH 1/2] =?UTF-8?q?test(cli):=20os=20migrate=20meta=20gets=20i?= =?UTF-8?q?ts=20end-to-end=20proof=20=E2=80=94=20the=20real=20CLI=20over?= =?UTF-8?q?=20a=20pre-17=20project?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01QuViRSR1j6GJjf9qGbnqFX --- packages/cli/test/migrate-meta.e2e.test.ts | 245 +++++++++++++++++++++ 1 file changed, 245 insertions(+) create mode 100644 packages/cli/test/migrate-meta.e2e.test.ts diff --git a/packages/cli/test/migrate-meta.e2e.test.ts b/packages/cli/test/migrate-meta.e2e.test.ts new file mode 100644 index 0000000000..b32893d1c9 --- /dev/null +++ b/packages/cli/test/migrate-meta.e2e.test.ts @@ -0,0 +1,245 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `os migrate meta` — end-to-end over the REAL CLI process (ADR-0087 D3/D5). + * + * The chain's transforms are fixture-tested in @objectstack/spec + * (conversions.test.ts, migrations.test.ts), but the COMMAND path — config + * loading via bundleRequire, the convert:false normalization that keeps the + * chain attributable, the schema-validity verdict, the machine JSON shape, + * `--out` snapshots, and the support-floor refusal — had no test at all. + * This spawns `bin/run-dev.js` against a temp project authored in the + * PRE-protocol-17 dialect, exercising one key from every v17 conversion + * family in a single pass, exactly the cross-major "consumer upgrades" + * scenario the command exists for. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { execFile } from 'node:child_process'; +import { promisify } from 'node:util'; +import { mkdtempSync, rmSync, writeFileSync, readFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { ObjectStackDefinitionSchema } from '@objectstack/spec'; + +const execFileP = promisify(execFile); +const HERE = resolve(fileURLToPath(import.meta.url), '..'); +const CLI = resolve(HERE, '../bin/run-dev.js'); +const TSX = resolve(HERE, '../../../node_modules/.bin/tsx'); + +/** + * A stack authored against protocol 16: every line marked `// 16:` is a shape + * the v17 chain must rewrite, spanning each conversion family — renames + * (action execute→target, sharing full→edit), the required→storage.notNull + * explicitization, and the #3896 close-out removals (rls.priority, the four + * tool keys, flow active/template/outputSchema/fallbackNodeId, view/dashboard + * inert keys, agent.knowledge, skill.triggerPhrases). + */ +const PRE17_CONFIG = ` +export default { + name: 'migrate_meta_e2e', + label: 'Migrate Meta E2E', + objects: [{ + name: 'e2e_ticket', + label: 'Ticket', + fields: { + title: { type: 'text', label: 'Title', required: true }, // 16: required implied NOT NULL + notes: { type: 'textarea', label: 'Notes' }, + }, + }], + actions: [{ + name: 'close_ticket', + label: 'Close', + type: 'script', + execute: 'closeHandler', // 16: renamed to target + shortcut: 'Ctrl+K', // 16: removed (never dispatched) + bulkEnabled: true, // 16: removed (toolbar reads view bulkActions) + }], + flows: [{ + name: 'e2e_flow', + label: 'E2E Flow', + type: 'autolaunched', + active: false, // 16: removed (status is the lifecycle) + template: true, // 16: removed (no reader) + errorHandling: { strategy: 'retry', fallbackNodeId: 'n9' }, // 16: fault edges own this + nodes: [ + { id: 'n1', type: 'start', label: 'Start', outputSchema: { ok: { type: 'boolean' } } }, // 16: removed + { id: 'n2', type: 'delete_record', label: 'Purge', config: { objectName: 'e2e_ticket', filter: { done: true } } }, // canonical since protocol 11 — must pass through untouched + ], + edges: [], + }], + views: [{ + object: 'e2e_ticket', + list: { type: 'grid', columns: ['title'], responsive: { hiddenOn: ['xs'] }, performance: { lazyLoad: true } }, // 16: removed + form: { type: 'simple', sections: [{ fields: ['title'] }], defaultSort: [{ field: 'title' }], aria: { label: 'Ticket' } }, // 16: removed + }], + dashboards: [{ + name: 'e2e_kpis', + label: 'E2E KPIs', + aria: { label: 'KPIs' }, // 16: removed + performance: { prefetch: true },// 16: removed + widgets: [], + }], + agents: [{ + name: 'e2e_agent', + label: 'Agent', + role: 'Helper', + instructions: 'help', + knowledge: { sources: ['faq'] }, // 16: removed (never scoped retrieval) + }], + skills: [{ + name: 'e2e_skill', + label: 'Skill', + tools: ['query_records'], + triggerPhrases: ['do the thing'], // 16: removed (never matched) + }], + tools: [{ + name: 'e2e_tool', + label: 'Tool', + description: 'A tool', + parameters: { type: 'object' }, + category: 'action', // 16: removed + permissions: ['x.y'], // 16: removed (gated nothing) + active: true, // 16: removed (withdrew nothing) + builtIn: false, // 16: removed + }], + permissions: [{ + name: 'e2e_set', + label: 'Set', + objects: { e2e_ticket: { allowRead: true } }, + rowLevelSecurity: [{ + name: 'own_rows', + object: 'e2e_ticket', + operation: 'select', + using: 'owner = current_user.id', + enabled: true, + priority: 10, // 16: removed (no conflict to order) + }], + }], + sharingRules: [{ + name: 'share_hot', + type: 'criteria', + object: 'e2e_ticket', + label: 'Hot tickets', + condition: "record.hot == true", + sharedWith: { type: 'team', value: 'support' }, + accessLevel: 'full', // 16: full→edit (walked at the TOP-LEVEL collection) + }], +}; +`; + +/** Conversion ids the run MUST attribute at least one rewrite to. */ +const EXPECTED_CONVERSIONS = [ + 'action-execute-to-target', + 'action-inert-keys-removed', + 'flow-inert-keys-removed', + 'view-inert-keys-removed', + 'dashboard-inert-keys-removed', + 'agent-knowledge-removed', + 'skill-trigger-phrases-removed', + 'tool-inert-authoring-keys-removed', + 'permission-rls-priority-removed', + 'field-required-notnull-explicit', + 'sharing-rule-access-level-full-to-edit', +]; + +let dir: string; +let out: { stdout: string; parsed: any }; + +async function runMeta(args: string[], cwd: string) { + const { stdout } = await execFileP(TSX, [CLI, 'migrate', 'meta', ...args], { + cwd, + maxBuffer: 16 * 1024 * 1024, + env: { ...process.env, NO_COLOR: '1' }, + }); + return stdout; +} + +beforeAll(async () => { + dir = mkdtempSync(join(tmpdir(), 'os-migrate-meta-e2e-')); + writeFileSync(join(dir, 'objectstack.config.ts'), PRE17_CONFIG); + const stdout = await runMeta(['--from', '16', '--json', '--out', join(dir, 'migrated.json')], dir); + out = { stdout, parsed: JSON.parse(stdout) }; +}, 120_000); + +afterAll(() => { + try { rmSync(dir, { recursive: true, force: true }); } catch { /* ignore */ } +}); + +describe('os migrate meta --from 16 (e2e over the real CLI)', () => { + it('replays the v17 chain and reports schema-valid output', () => { + expect(out.parsed.from).toBe(16); + expect(out.parsed.schemaValid).toBe(true); + }); + + it('attributes at least one rewrite to every expected v17 conversion family', () => { + const ids = new Set(out.parsed.applied.map((a: any) => a.conversionId)); + for (const id of EXPECTED_CONVERSIONS) { + expect(ids.has(id), `expected a rewrite from ${id}; got ${[...ids].join(', ')}`).toBe(true); + } + }); + + it('surfaces the semantic TODOs instead of auto-applying them', () => { + expect(Array.isArray(out.parsed.todos)).toBe(true); + expect(out.parsed.todos.length).toBeGreaterThan(0); + for (const t of out.parsed.todos) { + expect(t.reason?.length).toBeGreaterThan(0); + expect(t.acceptanceCriteria?.length).toBeGreaterThan(0); + } + }); + + it('the --out snapshot re-parses under the CURRENT schema and carries the rewrites', () => { + const snap = JSON.parse(readFileSync(join(dir, 'migrated.json'), 'utf-8')); + const parsed = ObjectStackDefinitionSchema.safeParse(snap); + expect(parsed.success, JSON.stringify(parsed.success ? '' : parsed.error.issues.slice(0, 3))).toBe(true); + + // Spot-check one rewrite per family on the snapshot itself. + expect(snap.actions[0].target).toBe('closeHandler'); + expect(snap.actions[0].execute).toBeUndefined(); + expect(snap.actions[0].shortcut).toBeUndefined(); + expect(snap.flows[0].active).toBeUndefined(); + expect(snap.flows[0].errorHandling.fallbackNodeId).toBeUndefined(); + expect(snap.flows[0].nodes[0].outputSchema).toBeUndefined(); + expect(snap.flows[0].nodes[1].config.filter, 'canonical key passes through untouched').toEqual({ done: true }); + expect(snap.views[0].list.responsive).toBeUndefined(); + expect(snap.views[0].form.defaultSort).toBeUndefined(); + expect(snap.dashboards[0].aria).toBeUndefined(); + expect(snap.agents[0].knowledge).toBeUndefined(); + expect(snap.skills[0].triggerPhrases).toBeUndefined(); + expect(snap.tools[0].category).toBeUndefined(); + expect(snap.permissions[0].rowLevelSecurity[0].priority).toBeUndefined(); + expect(snap.sharingRules[0].accessLevel).toBe('edit'); + // ADR-0113 explicitization: the pre-17 required field carries its column + // constraint in writing; the optional field gains nothing. + expect(snap.objects[0].fields.title.storage).toEqual({ notNull: true }); + expect(snap.objects[0].fields.notes.storage).toBeUndefined(); + }); + + it('is idempotent: replaying the chain on the migrated snapshot applies zero changes', async () => { + const dir2 = mkdtempSync(join(tmpdir(), 'os-migrate-meta-e2e2-')); + try { + const snap = readFileSync(join(dir, 'migrated.json'), 'utf-8'); + writeFileSync(join(dir2, 'objectstack.config.ts'), `export default ${snap};`); + const stdout = await runMeta(['--from', '16', '--json'], dir2); + const again = JSON.parse(stdout); + expect(again.applied).toEqual([]); + expect(again.schemaValid).toBe(true); + } finally { + try { rmSync(dir2, { recursive: true, force: true }); } catch { /* ignore */ } + } + }, 120_000); + + it('refuses a --from below the support floor with the structured error', async () => { + let failed = false; + try { + await runMeta(['--from', '9', '--json'], dir); + } catch (e: any) { + failed = true; + const body = JSON.parse(String(e.stdout || '{}')); + expect(body.error).toBe('unsupported_from_major'); + expect(body.message).toMatch(/floor|support/i); + } + expect(failed, 'expected a non-zero exit below the floor').toBe(true); + }, 120_000); +}); From 5dc54a62b2ff5bd30f474cc756071ed9b041216f Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 07:30:52 +0000 Subject: [PATCH 2/2] fix(sharing): inert-rule warn dedup + boot aggregate (#3929 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01QuViRSR1j6GJjf9qGbnqFX --- .changeset/inert-rule-warn-dedupe.md | 13 +++++++ .../plugin-sharing/src/boot-backfill.test.ts | 38 +++++++++++++++++++ .../plugin-sharing/src/sharing-plugin.ts | 11 ++++++ .../src/sharing-rule-service.ts | 27 +++++++++++-- 4 files changed, 85 insertions(+), 4 deletions(-) create mode 100644 .changeset/inert-rule-warn-dedupe.md diff --git a/.changeset/inert-rule-warn-dedupe.md b/.changeset/inert-rule-warn-dedupe.md new file mode 100644 index 0000000000..7859f8840c --- /dev/null +++ b/.changeset/inert-rule-warn-dedupe.md @@ -0,0 +1,13 @@ +--- +"@objectstack/plugin-sharing": patch +--- + +fix(sharing): the criteria-less-rule warn is once per rule per process, plus one boot aggregate (#3929 follow-up) + +Pre-dedup the fail-closed evaluator warned on EVERY pass — per evaluation and +per reconciled write — so one legacy criteria-less rule could dominate a +deployment's log. Enforcement is unchanged (such a rule still matches +nothing and its grants are revoked on reconcile); the warn now fires once +per rule per process, and the boot backfill emits a single operator-facing +aggregate (count + rule names + the fix: repair the criteria or set +active: false). diff --git a/packages/plugins/plugin-sharing/src/boot-backfill.test.ts b/packages/plugins/plugin-sharing/src/boot-backfill.test.ts index 96000170f5..0fea98f84e 100644 --- a/packages/plugins/plugin-sharing/src/boot-backfill.test.ts +++ b/packages/plugins/plugin-sharing/src/boot-backfill.test.ts @@ -206,3 +206,41 @@ describe("backfillRetiredAccessLevels (#3865 — stored 'full' normalises to 'ed expect(engine._tables.sys_record_share[0].access_level).toBe('full'); }); }); + +describe('#3929 follow-up — inert-rule warn dedup + boot aggregate', () => { + it('warns ONCE per inert rule per process, and the boot pass emits one aggregate', async () => { + const engine = makeEngine(); + const sharing = new SharingService({ engine: engine as any }); + const warn = vi.fn(); + const rules = new SharingRuleService({ engine: engine as any, sharing, logger: { warn } as any }); + + engine._tables.sys_sharing_rule = [ + { id: 'r_legacy', name: 'legacy_match_all', object_name: 'showcase_inquiry', criteria: null, recipient_type: 'position', recipient_id: 'p1', access_level: 'read', active: true }, + { id: 'r_legacy2', name: 'legacy_match_all_2', object_name: 'showcase_inquiry', criteria: {}, recipient_type: 'position', recipient_id: 'p1', access_level: 'read', active: true }, + ]; + engine._tables.showcase_inquiry = [{ id: 'inq_1', status: 'new' }]; + + // Repeated evaluations — the pre-dedup behavior warned on every one. + await rules.evaluateRule('r_legacy', SYS); + await rules.evaluateRule('r_legacy', SYS); + await rules.evaluateRule('r_legacy2', SYS); + await rules.evaluateRule('r_legacy', SYS); + + const perRule = warn.mock.calls.filter(([msg]) => String(msg).includes('no usable criteria')); + expect(perRule).toHaveLength(2); // once per DISTINCT rule, not per pass + expect(perRule.map(([, meta]) => (meta as any).rule).sort()).toEqual(['legacy_match_all', 'legacy_match_all_2']); + + // Enforcement unchanged on every pass: nothing granted, ever. + expect(engine._tables.sys_record_share ?? []).toHaveLength(0); + + // The boot backfill walks the same rules and adds ONE aggregate line. + const bootWarn = vi.fn(); + await backfillRuleGrants(rules, engine._tables.sys_sharing_rule, { warn: bootWarn }); + const agg = bootWarn.mock.calls.filter(([msg]) => String(msg).includes('matching NO records')); + expect(agg).toHaveLength(1); + expect((agg[0][1] as any).count).toBe(2); + expect((agg[0][1] as any).rules.sort()).toEqual(['r_legacy', 'r_legacy2']); + // …and the per-rule warns did NOT repeat during the backfill pass. + expect(warn.mock.calls.filter(([msg]) => String(msg).includes('no usable criteria'))).toHaveLength(2); + }); +}); diff --git a/packages/plugins/plugin-sharing/src/sharing-plugin.ts b/packages/plugins/plugin-sharing/src/sharing-plugin.ts index 0eda46d1a9..6ea9378a13 100644 --- a/packages/plugins/plugin-sharing/src/sharing-plugin.ts +++ b/packages/plugins/plugin-sharing/src/sharing-plugin.ts @@ -72,6 +72,17 @@ export async function backfillRuleGrants( ms: Date.now() - start, }); } + // [#3929 follow-up] One aggregate line for the legacy criteria-less rules + // the pass just walked (each also warned once, above, via the per-rule + // dedup): the operator-facing summary of what is under-sharing and why. + const inert = ruleService.inertRuleNames; + if (inert.length > 0) { + logger?.warn?.( + 'SharingServicePlugin: rule(s) with no usable criteria are matching NO records — their ' + + 'grants are revoked on reconcile (ADR-0049). Fix the criteria or set active: false.', + { count: inert.length, rules: inert }, + ); + } return reconciled; } diff --git a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts index f3632edbc3..84a3678b4c 100644 --- a/packages/plugins/plugin-sharing/src/sharing-rule-service.ts +++ b/packages/plugins/plugin-sharing/src/sharing-rule-service.ts @@ -67,6 +67,15 @@ export class SharingRuleService implements ISharingRuleService { private readonly engine: SharingEngine; private readonly sharing: SharingService; private readonly logger?: SharingRuleServiceOptions['logger']; + /** + * [#3929 follow-up] Inert (criteria-less) rules seen this process, for + * once-per-rule warn dedup + the boot aggregate. Pre-dedup the evaluator + * warned on EVERY pass — findMatchingRecords per evaluation AND + * recordMatches per reconciled write — so one legacy row could dominate a + * deployment's log. The enforcement is unchanged (such a rule still + * matches NOTHING); only the repetition is gone. + */ + private readonly inertRuleSeen = new Set(); constructor(opts: SharingRuleServiceOptions) { this.engine = opts.engine; @@ -301,13 +310,23 @@ export class SharingRuleService implements ISharingRuleService { */ private isInertMatchAll(rule: SharingRuleRow): boolean { if (!isMatchAllCriteria(rule.criteria)) return false; - this.logger?.warn?.( - '[sharing-rule] rule has no usable criteria — matching NO records instead of every record (ADR-0049)', - { rule: rule.name, object: rule.object_name }, - ); + const key = String(rule.id ?? rule.name); + if (!this.inertRuleSeen.has(key)) { + this.inertRuleSeen.add(key); + this.logger?.warn?.( + '[sharing-rule] rule has no usable criteria — matching NO records instead of every record ' + + '(ADR-0049; logged once per rule per process — fix the criteria or set active: false)', + { rule: rule.name, object: rule.object_name }, + ); + } return true; } + /** Names of inert (criteria-less) rules seen so far — the boot aggregate reads this. */ + get inertRuleNames(): readonly string[] { + return [...this.inertRuleSeen]; + } + private async findMatchingRecords(rule: SharingRuleRow): Promise { if (this.isInertMatchAll(rule)) return []; const filter = (rule.criteria ?? {}) as any;