fix(plugin-security): 派生 capability 只 reconcile 自己那半边的行,admin/package 行不再每 boot 被占位符覆盖 (#5876)

.changeset/derived-capability-managed-by-guard.md

@@ -0,0 +1,43 @@
+---
+'@objectstack/plugin-security': patch
+---
+
+**An admin-authored capability's `label`/`description` survive the boot (#5876).**
+
+`bootstrapSystemCapabilities` seeds `sys_capability` in two halves: the CURATED
+platform capabilities, and the back-compat DERIVED defaults — one row per
+capability string a bootstrap permission set grants via `systemPermissions[]`
+that nothing declared. Its seed loop refreshed `label`/`description` on whatever
+row it found for a name, without looking at `managed_by`, while the comment
+directly above it claimed the opposite ("do NOT clobber admin edits"). What
+#2909 T3 actually made seed-once is `scope`, and only `scope`.
+
+For a derived name there is no authored copy to reconcile: `label` is
+`humanize(name)` and `description` is `Capability <name>.`, both generated from
+the granted string. So an existing row's authored display fields were rewritten
+to a humanized placeholder on **every boot**, whoever wrote them — silent data
+loss, invisible from the outside.
+
+Reachable, narrowly, and it needs the admin row to pre-exist the grant: an admin
+creates capability `X` in Setup (`managed_by:'admin'` — the only provenance the
+ADR-0066 write-guard leaves admin-writable), an app whose bootstrap permission
+set grants `X` is installed, and every boot from then on renames it. The reverse
+order is not reachable: once the derivation has created the
+`managed_by:'platform'` placeholder, the write-guard stops the admin editing it
+at all.
+
+**The derived half now reconciles display fields only on rows it owns** —
+`managed_by:'platform'` on a non-curated name, which can only be its own
+placeholder from an earlier boot. `admin` rows, `package` rows and rows whose
+provenance is missing are left exactly as their author wrote them, and counted
+in the new `skippedAuthored` field of the seeding result (reported in the boot
+summary, not warned about: nothing is degraded, the capability resolves and the
+authored copy is the better one).
+
+**The curated half is unchanged.** Those definitions are authored by the
+platform and a new version legitimately ships new copy, so a curated name still
+refreshes the row it finds. `scope` stays seed-once on both halves.
+
+No migration and no authoring change: a placeholder that was already
+overwritten is not restored (the previous text is gone), but it stops being
+overwritten again, and an admin's re-edit now sticks.

packages/plugins/plugin-security/src/bootstrap-declared-capabilities.test.ts

@@ -214,14 +214,38 @@ describe('refused declarations vs. the derived placeholder (#4967 Part 1)', () =
     });
   });
 
-  it('REVERSE: dropping that name from the list lets the derivation overwrite the admin row', async () => {
-    // Why the unowned path checks for an EXISTING row instead of always
-    // falling through: the derived defaults refresh label/description on any
-    // row they find.
+  it('SECOND LAYER (#5876): dropping that name from the list no longer overwrites the admin row', async () => {
+    // This pin was written by #5875 (as `REVERSE: dropping that name from the
+    // list lets the derivation overwrite the admin row`) to justify why the
+    // unowned path checks for an EXISTING row instead of always falling
+    // through: back then the derived defaults refreshed label/description on
+    // ANY row they found, so the skip list was the ONLY thing standing between
+    // an admin-authored row and a humanized placeholder. That fact is fixed —
+    // #5876 guards the derived reconcile by `managed_by`, so the write site
+    // now refuses the clobber even when the call site forgets to suppress it.
+    //
+    // The suppression list keeps its job (it is what states the boot-order
+    // contract, and it stops the derivation doing work on names another pass
+    // owns), but it is no longer load-bearing for THIS shape — which is the
+    // point of a second layer.
     const ql = makeQl([]);
     ql.rows.push({ id: 'cap_admin', name: 'showcase.export_data', label: 'Admin Made', description: 'Admin wrote this.', managed_by: 'admin' });
     await bootstrapSystemCapabilities(ql, OPS_SETS, { materializedCapabilityNames: [] });
-    expect(ql.rows.find((r) => r.name === 'showcase.export_data')?.label).toBe('Showcase Export Data');
+    expect(ql.rows.find((r) => r.name === 'showcase.export_data')).toMatchObject({
+      label: 'Admin Made', description: 'Admin wrote this.', managed_by: 'admin',
+    });
+  });
+
+  it('DISCRIMINATION (#5876): with the name dropped, a PLATFORM placeholder is still refreshed', async () => {
+    // The guard above must not read as "the derivation stopped writing". Same
+    // fixture, same empty skip list, provenance flipped to the one the derived
+    // pass owns → the reconcile happens exactly as it always did.
+    const ql = makeQl([]);
+    ql.rows.push({ id: 'cap_derived', name: 'showcase.export_data', label: 'Stale Placeholder', description: 'Stale.', managed_by: 'platform' });
+    await bootstrapSystemCapabilities(ql, OPS_SETS, { materializedCapabilityNames: [] });
+    expect(ql.rows.find((r) => r.name === 'showcase.export_data')).toMatchObject({
+      label: 'Showcase Export Data', description: 'Capability showcase.export_data.', managed_by: 'platform',
+    });
   });
 
   it('FOREIGN owner: suppresses, because the other package authored that row', async () => {

packages/plugins/plugin-security/src/bootstrap-system-capabilities.test.ts

@@ -94,3 +94,117 @@ describe('bootstrapSystemCapabilities (ADR-0066 D1 back-compat seed)', () => {
     expect(KNOWN_CAPABILITIES.filter((c) => c.scope === 'platform').length).toBeGreaterThanOrEqual(5);
   });
 });
+
+// ───────────────────────────────────────────────────────────────────────────
+// [#5876] The DERIVED half reconciles display fields only on rows it OWNS.
+//
+// The seed loop used to refresh `label`/`description` on whatever row it found
+// for a derived name, whatever its provenance — while the comment above it said
+// admin edits were not clobbered. For a derived name `label` is `humanize(name)`
+// and `description` is `Capability <name>.`, so an admin-authored row was
+// rewritten to a humanized placeholder on EVERY boot (silent data loss; the
+// reachable chain is: admin creates the capability in Setup → an app whose
+// bootstrap permission set grants it by name is installed → every boot after).
+//
+// The CURATED half keeps refreshing (the platform ships new copy for its own
+// definitions — pinned by 'does NOT clobber an admin-edited scope on re-seed'
+// above); only the derived half is guarded, so these pins must DISCRIMINATE
+// rather than just prove nothing is written.
+// ───────────────────────────────────────────────────────────────────────────
+describe('derived defaults never clobber an authored row (#5876)', () => {
+  const OPS_SETS = [{ systemPermissions: ['showcase.export_data'] }];
+  const AUTHORED = { label: 'Admin Made', description: 'Admin wrote this.' };
+
+  /** A pre-existing row for a name the derivation would otherwise derive. */
+  function seedRow(ql: ReturnType<typeof makeQl>, managed_by: string | undefined) {
+    ql.rows.push({
+      id: 'cap_existing',
+      name: 'showcase.export_data',
+      ...AUTHORED,
+      scope: 'org',
+      ...(managed_by === undefined ? {} : { managed_by }),
+      active: true,
+    });
+  }
+
+  it('leaves an ADMIN-authored row untouched', async () => {
+    const ql = makeQl();
+    seedRow(ql, 'admin');
+    const out = await bootstrapSystemCapabilities(ql, OPS_SETS);
+    expect(ql.rows.find((r) => r.name === 'showcase.export_data')).toMatchObject({
+      ...AUTHORED, managed_by: 'admin', scope: 'org',
+    });
+    expect(out.skippedAuthored).toBe(1);
+    // The skip is a SKIP, not a silent failed write: it is not counted as an update.
+    expect(ql.rows.filter((r) => r.name === 'showcase.export_data')).toHaveLength(1);
+  });
+
+  it('leaves a PACKAGE-authored row untouched', async () => {
+    const ql = makeQl();
+    ql.rows.push({
+      id: 'cap_pkg', name: 'showcase.export_data', label: 'Export Data', description: 'Bulk export.',
+      managed_by: 'package', package_id: 'com.acme.reports', active: true,
+    });
+    const out = await bootstrapSystemCapabilities(ql, OPS_SETS);
+    expect(ql.rows.find((r) => r.name === 'showcase.export_data')).toMatchObject({
+      label: 'Export Data', description: 'Bulk export.', managed_by: 'package', package_id: 'com.acme.reports',
+    });
+    expect(out.skippedAuthored).toBe(1);
+  });
+
+  it('leaves a row of UNKNOWN provenance untouched (the field defaults to admin)', async () => {
+    // `sys_capability.managed_by` is required with `defaultValue: 'admin'`, so a
+    // row that reaches this pass without one is not a platform placeholder —
+    // "not provably ours" resolves to leave-it-alone, never to overwrite.
+    const ql = makeQl();
+    seedRow(ql, undefined);
+    const out = await bootstrapSystemCapabilities(ql, OPS_SETS);
+    expect(ql.rows.find((r) => r.name === 'showcase.export_data')).toMatchObject(AUTHORED);
+    expect(out.skippedAuthored).toBe(1);
+  });
+
+  it('POSITIVE CONTROL: still refreshes its OWN platform placeholder', async () => {
+    // Same fixture, same grant — only the provenance differs. A guard that also
+    // switched this case off would be indistinguishable from deleting the
+    // reconcile, so this is what gives the three pins above their teeth.
+    const ql = makeQl();
+    seedRow(ql, 'platform');
+    const out = await bootstrapSystemCapabilities(ql, OPS_SETS);
+    expect(ql.rows.find((r) => r.name === 'showcase.export_data')).toMatchObject({
+      label: 'Showcase Export Data', description: 'Capability showcase.export_data.', managed_by: 'platform',
+    });
+    expect(out.skippedAuthored).toBe(0);
+    expect(out.updated).toBeGreaterThanOrEqual(1);
+    // [#2909 T3] `scope` stays seed-once even on a row this pass owns.
+    expect(ql.rows.find((r) => r.name === 'showcase.export_data')?.scope).toBe('org');
+  });
+
+  it('stays stable across boots: derive, then never re-write the row again', async () => {
+    const ql = makeQl();
+    const boot1 = await bootstrapSystemCapabilities(ql, OPS_SETS);
+    expect(boot1.seeded).toBe(KNOWN_CAPABILITIES.length + 1);
+    // An admin renames the derived placeholder… which the platform/package write
+    // guard actually refuses today (see #5876's reachability note), so simulate
+    // the storage effect only, and re-boot.
+    const row = ql.rows.find((r) => r.name === 'showcase.export_data')!;
+    row.label = 'Renamed By Admin';
+    row.managed_by = 'admin';
+    const boot2 = await bootstrapSystemCapabilities(ql, OPS_SETS);
+    expect(row.label).toBe('Renamed By Admin');
+    expect(boot2.seeded).toBe(0);
+    expect(boot2.skippedAuthored).toBe(1);
+  });
+
+  it('the guard is scoped to the DERIVED half — curated names still refresh', async () => {
+    const ql = makeQl();
+    await bootstrapSystemCapabilities(ql, []);
+    const curated = KNOWN_CAPABILITIES[0];
+    const row = ql.rows.find((r) => r.name === curated.name)!;
+    row.label = 'stale label';
+    row.description = 'stale description';
+    const out = await bootstrapSystemCapabilities(ql, []);
+    expect(row.label).toBe(curated.label);
+    expect(row.description).toBe(curated.description);
+    expect(out.skippedAuthored).toBe(0);
+  });
+});

packages/plugins/plugin-security/src/bootstrap-system-capabilities.ts

@@ -18,6 +18,18 @@
  * seeding). Platform-seeded rows are `managed_by: 'platform'` so they are not
  * presented as admin-deletable. Runs on `kernel:ready` alongside the other
  * security bootstraps.
+ *
+ * [#5876] The two halves have DIFFERENT authority over an existing row's
+ * display fields, because they have different claims to authorship:
+ *   - CURATED — the platform authored `label`/`description`, and a new version
+ *     may ship new copy, so the row it finds is refreshed;
+ *   - DERIVED — there is no authored copy at all, only `humanize(name)` and
+ *     `Capability <name>.` generated from a granted string, so it refreshes
+ *     only its OWN placeholder (`managed_by:'platform'` on a non-curated name)
+ *     and never a row an admin or a package authored.
+ * The seed loop used to refresh both alike while the comment in front of it
+ * claimed admin edits were preserved — what #2909 T3 actually made seed-once is
+ * `scope`, and only `scope`.
  */
 
 import { PLATFORM_CAPABILITIES, type PlatformCapability } from '@objectstack/spec/security';
@@ -78,13 +90,31 @@ interface SeedOptions {
   materializedCapabilityNames?: Iterable<string>;
 }
 
+/** Aggregated outcome of a back-compat capability seeding pass. */
+export interface CapabilitySeedResult {
+  /** Rows inserted (curated definitions + derived placeholders). */
+  seeded: number;
+  /** Rows whose platform display fields were reconciled. */
+  updated: number;
+  /**
+   * [#5876] Derived names whose existing row is authored elsewhere
+   * (`managed_by` anything but `'platform'`), so its `label`/`description` were
+   * left as their author wrote them. Not a degradation — the capability
+   * resolves and the authored copy is the better one — so it is reported in the
+   * boot summary rather than warned about (#4632).
+   */
+  skippedAuthored: number;
+  /** Definitions considered this pass (curated + derived). */
+  total: number;
+}
+
 export async function bootstrapSystemCapabilities(
   ql: any,
   permissionSets: Array<{ systemPermissions?: string[] }> = [],
   options: SeedOptions = {},
-): Promise<{ seeded: number; updated: number; total: number }> {
+): Promise<CapabilitySeedResult> {
   if (!ql || typeof ql.find !== 'function' || typeof ql.insert !== 'function') {
-    return { seeded: 0, updated: 0, total: 0 };
+    return { seeded: 0, updated: 0, skippedAuthored: 0, total: 0 };
   }
 
   const materialized = new Set<string>(options.materializedCapabilityNames ?? []);
@@ -94,26 +124,58 @@ export async function bootstrapSystemCapabilities(
   // ones that already have a row, which the declared seeder owns.
   const byName = new Map<string, CapabilityDef>();
   for (const c of KNOWN_CAPABILITIES) byName.set(c.name, c);
+  // [#5876] Which names came from the DERIVED half. The two halves carry
+  // different authority over an existing row's display fields (see the
+  // reconcile guard below), and after this loop `byName` cannot tell them
+  // apart on its own.
+  const derivedNames = new Set<string>();
   for (const ps of permissionSets) {
     for (const cap of ps?.systemPermissions ?? []) {
       if (typeof cap === 'string' && cap && !byName.has(cap) && !materialized.has(cap)) {
         byName.set(cap, { name: cap, label: humanize(cap), description: `Capability ${cap}.`, scope: 'platform' });
+        derivedNames.add(cap);
       }
     }
   }
 
   let seeded = 0;
   let updated = 0;
+  let skippedAuthored = 0;
   for (const def of byName.values()) {
     const existing = await tryFind(ql, 'sys_capability', { name: def.name }, 1);
-    if (existing[0]?.id) {
-      // Keep label/description fresh, but do NOT clobber admin edits — only
-      // platform-owned display fields are reconciled. `scope` is an
-      // admin-editable classification face (plain select on sys_capability),
-      // so it is seed-once: written on insert, never refreshed (#2909 T3).
-      // A curated scope change in a new platform version needs a data
-      // migration — recorded in the ADR-0094 addendum.
-      if (await tryUpdate(ql, 'sys_capability', { id: existing[0].id, label: def.label, description: def.description })) {
+    const row = existing[0];
+    if (row?.id) {
+      // [#5876] Reconcile display fields only where THIS pass owns the copy.
+      //
+      // A DERIVED name has no authored copy to ship: `label` is `humanize(name)`
+      // and `description` is `Capability <name>.`, both generated from the
+      // string a permission set happened to grant. Refreshing those onto a row
+      // somebody else authored is not reconciliation, it is overwriting an
+      // author with a placeholder — every boot, silently. For a non-curated
+      // name a `managed_by:'platform'` row can only be this same derivation's
+      // placeholder from an earlier boot, so that is exactly the set of rows
+      // the derived half may refresh; `admin` (Setup-authored), `package`
+      // (declared by its owning package) and anything else are left alone.
+      //
+      // The CURATED half is unchanged: those definitions are authored by the
+      // platform and a new version legitimately ships new copy, so a curated
+      // name still refreshes the row it finds.
+      //
+      // NOTE this is the WRITE-side enforcement of the same rule
+      // `materializedCapabilityNames` states at the CALL site (#4967 Part 1):
+      // the caller says which names another pass already materialized, and
+      // this guard holds even when nothing said so — an admin row for a name
+      // no package ever declared is invisible to that list.
+      if (derivedNames.has(def.name) && row.managed_by !== 'platform') {
+        skippedAuthored += 1;
+        continue;
+      }
+      // Keep label/description fresh from the platform's own definition.
+      // `scope` is an admin-editable classification face (plain select on
+      // sys_capability), so it is seed-once: written on insert, never
+      // refreshed (#2909 T3). A curated scope change in a new platform version
+      // needs a data migration — recorded in the ADR-0094 addendum.
+      if (await tryUpdate(ql, 'sys_capability', { id: row.id, label: def.label, description: def.description })) {
         updated += 1;
       }
     } else {
@@ -129,6 +191,8 @@ export async function bootstrapSystemCapabilities(
       if (created) seeded += 1;
     }
   }
-  options.logger?.info?.('[security] system capabilities seeded into sys_capability (ADR-0066 D1)', { seeded, updated, total: byName.size });
-  return { seeded, updated, total: byName.size };
+  options.logger?.info?.('[security] system capabilities seeded into sys_capability (ADR-0066 D1)', {
+    seeded, updated, skippedAuthored, total: byName.size,
+  });
+  return { seeded, updated, skippedAuthored, total: byName.size };
 }