test(cli),fix(sharing): migrate-meta end-to-end proof + inert-rule warn dedup — the last two finishing touches

.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).

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);
+});

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);
+  });
+});

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;
 }
 

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<string>();
 
   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<string[]> {
     if (this.isInertMatchAll(rule)) return [];
     const filter = (rule.criteria ?? {}) as any;