fix(plugin-security): a capability name collision now reaches the author

.changeset/18023-capability-name-collision-diagnostic.md

@@ -0,0 +1,34 @@
+---
+"@objectstack/plugin-security": minor
+---
+
+A **capability name collision now reaches the author**. When a package declares a capability whose name a *different* package already owns, `bootstrapDeclaredCapabilities` refuses to write into that row — correct under ADR-0086 D4, and unchanged — but the refusal is no longer invisible (#18023).
+
+Measured on the pre-change tree, with a collision seeded and **no logger passed**:
+
+```
+skippedForeign                = 1     (the declaration was dropped)
+author-visible console lines  = 0     (log, info, warn, error, debug — all five)
+diagnostic records on outcome = undefined
+```
+
+The branch reported through `logger?.warn?.(…)` — optionally chained **twice** — so a caller that passed no logger produced no output at all, and a package's whole declared capability vanished with one internal counter incremented. This module's own header said such a row was "skipped loudly"; nothing about it was loud. Same case after the change:
+
+```
+skippedForeign                = 1     (unchanged — the skip is not what was wrong)
+author-visible console lines  = 1     warn: [security] [capability_name_collision] …
+diagnostic records on outcome = 1     { name, declaredBy, ownedBy, grantedBy, message, fix }
+```
+
+**What the author is told is axis-specific, and deliberately not a copy of the permission-set wording.** On that axis the entire declared set is not materialized and none of its permissions are in effect. Here the capability name still *resolves* — the owning package's row answers for it, and the seeder still reports the name as materialized so the back-compat derivation does not clobber that row. What is lost is narrower and is now stated precisely: the declaring package's authored `label`, `description` and `scope` are not applied, and `sys_capability.package_id` attributes the capability to the other package, so the declaring package has no provenance claim over it. The record also names the bootstrap permission set(s) that grant the capability, so the blast radius does not have to be looked up.
+
+New published surface on `@objectstack/plugin-security`, for the same reason the permission-set diagnostic is published — the author-time door must consume one derivation rather than re-spell it:
+
+- `CAPABILITY_NAME_COLLISION` — the stable `capability_name_collision` grep token.
+- `capabilityNameCollisionDiagnostic()` / `CapabilityNameCollisionDiagnostic` — the record.
+- `formatCapabilityNameCollisionDiagnostic()` — the one-line rendering.
+- `reportCapabilityNameCollisions()` — the report channel, which prints through `console.warn` when no sink is injected and keeps the receiver when one is, so a class-based host logger does not throw.
+
+⛔ **The owner-comparison predicate is not duplicated.** Both axes call the existing `permissionSetNameIsForeign`, and the capability seeder's branch now routes through it instead of its own `===`, so a nullish owner reads FOREIGN on both axes by construction.
+
+`CapabilitySeedOutcome` gains an optional `collisions` key carrying those records, so a caller that reads no log at all can still ask what happened. It is absent, never `[]`, when a pass collided on nothing.

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

@@ -1,8 +1,9 @@
 // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
 
-import { describe, it, expect } from 'vitest';
+import { describe, it, expect, vi } from 'vitest';
 import { bootstrapDeclaredCapabilities } from './bootstrap-declared-capabilities.js';
 import { bootstrapSystemCapabilities } from './bootstrap-system-capabilities.js';
+import { CAPABILITY_NAME_COLLISION } from './capability-name-collision.js';
 
 /** Minimal in-memory ql for sys_capability seeding with a registry stub. */
 function makeQl(declared: any[] = []) {
@@ -502,3 +503,205 @@ describe('#11518 — a truncated existence page must never route this seeder to
     expect(out.unreadable).toBe(0);
   });
 });
+
+/**
+ * [#18023] The foreign-owner refusal must REACH THE AUTHOR.
+ *
+ * ## What these pins are about, and what they deliberately do not touch
+ *
+ * ⛔ Not the skip. Refusing to write into a `sys_capability` row another
+ * package owns is correct under ADR-0086 D4 and is asserted UNCHANGED in every
+ * case below (`skippedForeign` still counts, the foreign row is never mutated,
+ * the name is still reported as materialized). The defect was that the refusal
+ * was invisible: `logger?.warn?.(…)` at that branch is optionally chained
+ * TWICE, so a caller passing no logger produced no output at all and an entire
+ * declared capability vanished with one internal counter moved.
+ *
+ * ## The measurement these replace
+ *
+ * On the pre-fix tree, driven through `bootstrapDeclaredCapabilities` with a
+ * foreign-owned row and NO logger, the first case below measured ZERO console
+ * lines across every channel and `undefined` for the outcome's diagnostic
+ * records, while `skippedForeign` was 1. That is the card's reading, reproduced.
+ *
+ * ## The discriminating half
+ *
+ * A pin that only asserts "something was printed on a collision" passes just as
+ * well against a seeder that prints on EVERY seeded capability, which is a
+ * different defect (#12015's: a diagnostic that fires always is as unreadable
+ * as one that never fires). So the no-collision control asserts SILENCE on all
+ * five console channels over a pass that really does seed, really does re-seed
+ * its own drifted row, and really does claim a derived placeholder.
+ *
+ * ⛔ These reuse this file's `makeQl` rather than standing up a second engine
+ * double: a new double would be a new row in `engine-double-contract`'s ledger
+ * for a card that names no engine seam.
+ */
+
+/**
+ * Capture EVERY console channel — "author-visible output" is not
+ * channel-specific, and a pin watching only `warn` could be satisfied by a
+ * change that merely MOVED the silence.
+ */
+function captureAllConsole() {
+  const seen: string[] = [];
+  const spies = (['log', 'info', 'warn', 'error', 'debug'] as const).map((m) =>
+    vi.spyOn(console, m).mockImplementation((...args: unknown[]) => {
+      seen.push(`${m}: ${args.map((a) => (typeof a === 'string' ? a : JSON.stringify(a))).join(' ')}`);
+    }),
+  );
+  return { seen, restore: () => spies.forEach((s) => s.mockRestore()) };
+}
+
+describe('[#18023] a capability-name collision reaches the author', () => {
+  it('prints, WITH NO LOGGER INJECTED, and returns the diagnostic record', async () => {
+    const sets = [{ name: 'ops', systemPermissions: ['shared_cap'] }];
+    const ql = makeQl([{ name: 'shared_cap', label: 'Mine', description: 'B wrote this.', scope: 'org', _packageId: 'com.example.b' }]);
+    ql.rows.push({
+      id: 'cap_owned_by_a',
+      name: 'shared_cap',
+      label: 'Owner Label',
+      description: 'Owner wrote this.',
+      scope: 'platform',
+      managed_by: 'package',
+      package_id: 'com.example.a',
+    });
+
+    const cap = captureAllConsole();
+    let out: Awaited<ReturnType<typeof bootstrapDeclaredCapabilities>>;
+    try {
+      // No logger — the exact call shape under which the old branch was mute.
+      out = await bootstrapDeclaredCapabilities(ql, null, { permissionSets: sets });
+    } finally {
+      cap.restore();
+    }
+
+    // ── The reading this card exists to move: 0 → 1 ──────────────────────────
+    expect(out.skippedForeign).toBe(1);
+    expect(cap.seen).toHaveLength(1);
+    expect(cap.seen[0]!.startsWith('warn: ')).toBe(true);
+    expect(cap.seen[0]).toContain(CAPABILITY_NAME_COLLISION);
+    expect(cap.seen[0]).toContain('shared_cap');
+
+    // ── The record travels back, for a caller that reads no log at all ──────
+    expect(out.collisions).toHaveLength(1);
+    expect(out.collisions![0]).toMatchObject({
+      event: CAPABILITY_NAME_COLLISION,
+      severity: 'warning',
+      name: 'shared_cap',
+      declaredBy: 'com.example.b',
+      ownedBy: 'com.example.a',
+      grantedBy: ['ops'],
+    });
+
+    // ── ⛔ The skip itself is UNCHANGED (ADR-0086 D4) ────────────────────────
+    expect(ql.rows.find((r) => r.name === 'shared_cap')).toMatchObject({
+      label: 'Owner Label', description: 'Owner wrote this.', scope: 'platform', package_id: 'com.example.a',
+    });
+    expect(ql.rows.filter((r) => r.name === 'shared_cap')).toHaveLength(1);
+    // …and the name is still reported materialized, so the back-compat
+    // derivation does not clobber the owner's authored row (#4967 Part 1).
+    expect(out.materializedNames).toEqual(['shared_cap']);
+  });
+
+  it('a package-managed row with NO package_id is FOREIGN — the shared predicate, not a local `===`', async () => {
+    const ql = makeQl([{ name: 'unowned_row_cap', _packageId: 'com.example.b' }]);
+    ql.rows.push({ id: 'cap_no_owner', name: 'unowned_row_cap', managed_by: 'package' });
+
+    const cap = captureAllConsole();
+    let out: Awaited<ReturnType<typeof bootstrapDeclaredCapabilities>>;
+    try {
+      out = await bootstrapDeclaredCapabilities(ql, null);
+    } finally {
+      cap.restore();
+    }
+
+    // ADR-0086 D3: a row that cannot prove its owner is not ours to write.
+    expect(out.skippedForeign).toBe(1);
+    expect(out.updated).toBe(0);
+    expect(out.collisions![0]!.ownedBy).toBeNull();
+    // The RECORD words it; the pass line summarises and carries the record in
+    // its structured meta. ⛔ Neither prints `undefined` at an author.
+    expect(out.collisions![0]!.message).toContain('(a package-managed row with no package_id)');
+    expect(cap.seen).toHaveLength(1);
+    expect(cap.seen[0]).toContain('"ownedBy":null');
+    expect(cap.seen[0]).not.toContain('undefined');
+    expect(ql.rows.find((r) => r.name === 'unowned_row_cap')!.package_id).toBeUndefined();
+  });
+
+  it('says it ONCE per pass, with every dropped declaration in the record list', async () => {
+    const ql = makeQl([
+      { name: 'cap_one', _packageId: 'com.example.b' },
+      { name: 'cap_two', _packageId: 'com.example.b' },
+      { name: 'cap_three', _packageId: 'com.example.b' },
+    ]);
+    for (const [id, name] of [['c1', 'cap_one'], ['c2', 'cap_two'], ['c3', 'cap_three']] as const) {
+      ql.rows.push({ id, name, managed_by: 'package', package_id: 'com.example.a' });
+    }
+
+    const cap = captureAllConsole();
+    let out: Awaited<ReturnType<typeof bootstrapDeclaredCapabilities>>;
+    try {
+      out = await bootstrapDeclaredCapabilities(ql, null);
+    } finally {
+      cap.restore();
+    }
+
+    expect(out.skippedForeign).toBe(3);
+    // ⛔ Three lines would bury the remedy under the finding.
+    expect(cap.seen).toHaveLength(1);
+    expect(cap.seen[0]).toContain('3 declared capabilities were NOT applied');
+    expect(out.collisions!.map((d) => d.name)).toEqual(['cap_one', 'cap_two', 'cap_three']);
+  });
+
+  it('reports through an INJECTED logger and leaves the console alone', async () => {
+    const warn = vi.fn();
+    const ql = makeQl([{ name: 'shared_cap', _packageId: 'com.example.b' }]);
+    ql.rows.push({ id: 'cap_x', name: 'shared_cap', managed_by: 'package', package_id: 'com.example.a' });
+
+    const cap = captureAllConsole();
+    try {
+      await bootstrapDeclaredCapabilities(ql, null, { logger: { warn } });
+    } finally {
+      cap.restore();
+    }
+
+    expect(warn).toHaveBeenCalledTimes(1);
+    expect(warn.mock.calls[0]![0]).toContain(CAPABILITY_NAME_COLLISION);
+    expect(cap.seen).toEqual([]);
+  });
+
+  it('CONTROL: a pass that really works and collides on nothing is SILENT on all five channels', async () => {
+    // ⛔ The discriminating half. Without this, a seeder that warned on every
+    // declaration would satisfy every assertion above.
+    const ql = makeQl([
+      { name: 'b.fresh', _packageId: 'com.example.b' },                  // → seeded
+      { name: 'b.drifted', label: 'New Label', _packageId: 'com.example.b' }, // → updated
+      { name: 'b.derived', _packageId: 'com.example.b' },                // → claimed
+      { name: 'b.steady', _packageId: 'com.example.b' },                 // → unchanged
+    ]);
+    ql.rows.push({ id: 'd1', name: 'b.drifted', label: 'Old Label', description: 'Capability b.drifted.', scope: 'platform', managed_by: 'package', package_id: 'com.example.b' });
+    ql.rows.push({ id: 'd2', name: 'b.derived', managed_by: 'platform' });
+    // ⚠️ Exactly what a re-seed would write (`humanize('b.steady')` = 'B Steady'),
+    // so this row takes the `unchanged` arm — an EQUALITY test, not a presence
+    // test. A label that merely looked plausible would take `updated` instead
+    // and silently delete this control's fourth kind of work.
+    ql.rows.push({ id: 'd3', name: 'b.steady', label: 'B Steady', description: 'Capability b.steady.', scope: 'platform', managed_by: 'package', package_id: 'com.example.b' });
+
+    const cap = captureAllConsole();
+    let out: Awaited<ReturnType<typeof bootstrapDeclaredCapabilities>>;
+    try {
+      out = await bootstrapDeclaredCapabilities(ql, null);
+    } finally {
+      cap.restore();
+    }
+
+    // The pass really did all four kinds of work…
+    expect(out).toMatchObject({ seeded: 1, updated: 1, claimed: 1, unchanged: 1, skippedForeign: 0 });
+    // …and said nothing at all.
+    expect(cap.seen).toEqual([]);
+    // The key stays ABSENT rather than empty: `undefined` and `[]` would
+    // otherwise be the same fact to a caller inspecting the outcome.
+    expect(out.collisions).toBeUndefined();
+  });
+});

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

@@ -29,8 +29,12 @@
  *    which is the whole point: retire the implicit back-door;
  *  - IDEMPOTENT + UPGRADE-AWARE: a row this seeder owns (`managed_by:'package'`,
  *    same `package_id`) is re-seeded on every boot so the record always
- *    reflects the shipped declaration. Rows owned by a DIFFERENT package are
- *    skipped loudly;
+ *    reflects the shipped declaration. A row owned by a DIFFERENT package is
+ *    skipped, and [#18023] the skip now REACHES THE AUTHOR — one
+ *    `capability_name_collision` line per pass through
+ *    {@link reportCapabilityNameCollisions}, which prints even with no logger
+ *    injected, plus the records themselves on
+ *    {@link CapabilitySeedOutcome.collisions};
  *  - admin-authored rows (`managed_by:'admin'`) are NEVER clobbered.
  *
  * Runs on `kernel:ready` in `@objectstack/plugin-security` alongside the other
@@ -82,6 +86,19 @@ import {
 } from './per-organization-catalog.js';
 import { buildExistingByName, type ExistingByNameIndex } from './seed-name-lookup.js';
 import { readDeclared } from './bootstrap-declared-permissions.js';
+// [#18023] THE owner-comparison predicate, REUSED rather than re-derived. It
+// carries the axis it was first written for in its NAME, and is
+// axis-INDEPENDENT in its question — "is the standing row's owner a DIFFERENT
+// package from the one declaring this?" — which is exactly what this seeder's
+// foreign branch asks. One name for one derivation beats two names for one
+// derivation; re-spelling the comparison here is the drift
+// `permission-set-name-collision.ts` exists to prevent.
+import { permissionSetNameIsForeign } from './permission-set-name-collision.js';
+import {
+  capabilityNameCollisionDiagnostic,
+  reportCapabilityNameCollisions,
+  type CapabilityNameCollisionDiagnostic,
+} from './capability-name-collision.js';
 import { PLATFORM_CAPABILITY_NAMES } from '@objectstack/spec/security';
 
 /** The only shape this seeder reads off a permission set: who grants what. */
@@ -160,6 +177,21 @@ export interface CapabilitySeedOutcome {
    * list, so the derivation gets its own attempt.)
    */
   materializedNames: string[];
+  /**
+   * [#18023] The declarations this pass DROPPED because another package owns
+   * the capability name — one record per dropped declaration, the structured
+   * half of what {@link reportCapabilityNameCollisions} prints.
+   *
+   * ⚠️ Present only when the pass found at least one, exactly like its
+   * permission-set sibling: the absent key keeps a caller from reading an empty
+   * array as "this pass looked" on an outcome that predates the key.
+   *
+   * Here so a caller that reads NO LOG AT ALL — a boot report, a test — can
+   * still ask what happened. ⛔ It is not a substitute for the print: a record
+   * nobody reads is the defect this card removed, so the pass reports through
+   * the sink whether or not anyone inspects this.
+   */
+  collisions?: CapabilityNameCollisionDiagnostic[];
 }
 
 function humanize(name: string): string {
@@ -342,7 +374,14 @@ async function upsertPackageCapability(
   }
 
   if (existing.managed_by === 'package') {
-    if (existing.package_id === packageId) {
+    // [#18023] ⛔ NOT a hand-written `existing.package_id === packageId`. The
+    // owner comparison is THE shared predicate (see the import note): a second
+    // spelling of it is free to drift from the one a compile-time door asks,
+    // and a nullish owner in particular must read FOREIGN — a package-managed
+    // row with no `package_id` is the exact ADR-0086 D3 ambiguity, and adopting
+    // it on a name match would be a package writing into a record it cannot
+    // prove it owns.
+    if (!permissionSetNameIsForeign(existing.package_id, packageId)) {
       // Our own row — re-seed so it always reflects the shipped declaration.
       //
       // [#11096] ⚠️ Only when the stored row ACTUALLY DIFFERS. An unconditional
@@ -362,10 +401,30 @@ async function upsertPackageCapability(
         out.updated += 1;
       }
     } else {
+      // [#18023] The SKIP is unchanged and correct — ADR-0086 D4: a package
+      // never writes into a foreign record. What changed is that it is no
+      // longer invisible.
+      //
+      // ⛔ The old line did not refuse loudly, whatever "skipped loudly" in
+      // this module's header claimed: `logger?.warn?.(…)` is optionally chained
+      // TWICE, so a caller passing no logger produced NO OUTPUT AT ALL and an
+      // entire declared capability vanished with one internal counter moved.
+      // Measured on the pre-fix tree with no logger: `skippedForeign = 1`,
+      // author-visible console lines = 0 across all five channels.
+      //
+      // The report now goes through `reportCapabilityNameCollisions` at the end
+      // of the pass — ONCE, and loud with no sink injected (#10556:
+      // silent-by-declaration is rejected) — and the diagnostic RECORD travels
+      // back on the outcome so a caller that reads no log at all can still ask
+      // what happened. ⛔ The record is not a substitute for the print; it is
+      // the second half of it.
       out.skippedForeign += 1;
-      logger?.warn?.('[security] capability name owned by another package — skipped', {
-        name: cap.name, declaredBy: packageId, ownedBy: existing.package_id,
-      });
+      (out.collisions ??= []).push(capabilityNameCollisionDiagnostic({
+        name: String(cap.name),
+        declaredBy: packageId,
+        ownedBy: typeof existing.package_id === 'string' ? existing.package_id : null,
+        grantedBy: grantors,
+      }));
     }
     // Either way a package-authored row exists and must not be re-derived over.
     return true;
@@ -451,6 +510,10 @@ export async function bootstrapDeclaredCapabilities(
   // This seeder is organization-less today (see the lookup note above), so the
   // report carries no organization either.
   reportSeedWriteRefusals(options.logger, refusals);
+  // [#18023] Said once per pass, and said even when no logger was injected —
+  // the whole defect was that this refusal reached nobody. The records are
+  // already on the outcome (`out.collisions`), for a caller that reads no log.
+  reportCapabilityNameCollisions(options.logger, out.collisions ?? []);
   if (out.unreadable > 0) {
     // [#11096] Said ONCE with the count, like the sibling seeders: a per-name
     // warn on a database that is down is a log flood that buries its own