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

.changeset/17516-permission-set-collision-diagnostic.md

@@ -0,0 +1,29 @@
+---
+"@objectstack/plugin-security": minor
+---
+
+A **permission-set name collision now reaches the author**. When a package declares a permission set whose name a *different* package already owns, `bootstrapDeclaredPermissions` refuses to write into that row — correct under ADR-0086 D4, and unchanged — but the refusal is no longer invisible (#17516).
+
+Measured on the pre-change tree, with a collision seeded and **no logger passed**:
+
+```
+skippedForeign                 = 1     (the entire declared set 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 permission set vanished with one internal counter incremented. The comment there said *"refuse 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] [permission_set_name_collision] …
+diagnostic records on outcome  = 1     { name, declaredBy, ownedBy, message, fix }
+```
+
+- **It prints with no sink injected.** `reportPermissionSetNameCollisions` falls back to `console.warn`, per the #10556 ruling that silent-by-declaration is rejected — an injected host sink still replaces it rather than printing beside it. The call keeps the receiver (a property-access call, never a detached `logger.warn ?? console.warn`), so a class-based host sink does not throw.
+- **The refusal is also readable without a log.** `PermissionSeedOutcome` gains an optional `collisions` array carrying one diagnostic per dropped set — absent, never `[]`, when the pass hit none. A counter with no record is what made the drop undiagnosable.
+- **One derivation, so two doors cannot drift.** `permissionSetNameIsForeign`, `permissionSetNameCollisionDiagnostic` and `formatPermissionSetNameCollisionDiagnostic` are exported from the package entry so a compile-time door consumes them rather than re-deriving the predicate or re-spelling the wording — the shape #14553 established for `navigationContributions`. ⚠️ Only the **runtime** door ships here; the compile-time door (`os build` / `os validate`) lives in another package and is not part of this change.
+- **A stable, greppable token**, `permission_set_name_collision`, is stamped as `event` on every report. It is a snake_case data value, not an ADR-0112 error code: it is never routed to `error.code` and never reaches a wire refusal, the same discrimination the sibling `position_name_fold_grant` token already makes in this package.
+- **The branch comment's premise is corrected.** It claimed package-namespaced object api names make set-name collisions a packaging bug rather than a merge case. **ADR-0130 D1 falsifies that** — N packages may co-own one namespace — so a collision is a legal configuration that gets *more* common, not an error that should never happen. The diagnostic's `fix` text names both legal resolutions.
+
+⛔ **No wire byte moves and no skip changes.** The foreign row is still never written; `skippedForeign` still counts it; the ADR-0086 P2 publish materializer still returns its existing `permission set name is owned by another package` failure text. A non-colliding pass stays completely silent on all five console channels, asserted over a pass that really does seed and re-seed.

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

@@ -105,7 +105,16 @@ describe('bootstrapDeclaredPermissions (ADR-0086 D5)', () => {
     });
     expect(r.skippedForeign).toBe(1);
     expect(ql.rows[0].package_id).toBe('com.example.crm');
-    expect(warns.some((w) => String(w.m).includes('owned by another package'))).toBe(true);
+    // [#17516] Re-anchored from the old prose ('owned by another package') to
+    // the stable token the report now stamps. The substance this pin asserts is
+    // unchanged — the refusal is reported — but the token is what an operator
+    // greps and what the sibling doors key on, so prose drift can no longer
+    // quietly unpin it. The read-back half is asserted beside it: a counter
+    // with no record is what made this drop invisible.
+    expect(warns.some((w) => String(w.m).includes('permission_set_name_collision'))).toBe(true);
+    expect(r.collisions).toEqual([
+      expect.objectContaining({ name: 'crm_sales_rep', declaredBy: 'com.example.other', ownedBy: 'com.example.crm' }),
+    ]);
   });
 
   it('skips a declared set with no resolvable owning package (warned, not seeded)', async () => {

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

@@ -68,6 +68,12 @@ import {
   reportSeedWriteRefusals,
   type SeedWriteRefusals,
 } from './per-organization-catalog.js';
+import {
+  permissionSetNameCollisionDiagnostic,
+  permissionSetNameIsForeign,
+  reportPermissionSetNameCollisions,
+  type PermissionSetNameCollisionDiagnostic,
+} from './permission-set-name-collision.js';
 
 export type { PermissionSeedOutcome } from './permission-set-projection.js';
 
@@ -206,6 +212,14 @@ export async function upsertPackagePermissionSet(
      * materialized nothing).
      */
     refusals?: SeedWriteRefusals;
+    /**
+     * [#17516] Collects set-name collisions so the pass reports them ONCE
+     * instead of a line per dropped set. Passed by the boot catalog loop; the
+     * ADR-0086 P2 publish materializer passes nothing and the refusal is
+     * reported at the branch instead — ⛔ never dropped, which is the whole
+     * point of this card.
+     */
+    collisions?: PermissionSetNameCollisionDiagnostic[];
   },
 ): Promise<PermissionSeedOutcome> {
   const out: PermissionSeedOutcome = { seeded: 0, updated: 0, unchanged: 0, unreadable: 0, skippedEnvAuthored: 0, skippedForeign: 0 };
@@ -260,7 +274,7 @@ export async function upsertPackagePermissionSet(
   }
 
   if (existing.managed_by === 'package') {
-    if (existing.package_id === packageId) {
+    if (!permissionSetNameIsForeign(existing.package_id, packageId)) {
       // Our own row — re-seed so the record always reflects the shipped/published
       // declaration (idempotent; covers version bumps without bookkeeping).
       //
@@ -283,13 +297,40 @@ export async function upsertPackagePermissionSet(
         out.updated += 1;
       }
     } else {
-      // Package-namespaced object api names make set-name collisions a
-      // packaging bug, not a merge case — refuse loudly (ADR-0086 D4:
-      // a package never writes into a foreign record).
+      // [#17516] 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 premise this branch used to state — "Package-namespaced object
+      // api names make set-name collisions a packaging bug, not a merge case"
+      // — is FALSIFIED by ADR-0130 D1, which lets N packages co-own one
+      // namespace (the ADR records it under "What was NOT decided"). A
+      // collision is therefore a legal configuration that gets MORE common as
+      // co-ownership lands, not a packaging error that should never happen. So
+      // the author reading it is the normal case, not the pathological one.
+      //
+      // ⛔ And the old line did not refuse loudly, whatever it claimed:
+      // `logger?.warn?.(…)` is optionally chained TWICE, so a caller passing no
+      // logger produced NO OUTPUT AT ALL and an entire declared permission set
+      // disappeared with one counter moved. The report now goes through
+      // `reportPermissionSetNameCollisions`, which prints 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 — a
+      // boot report, a test — can still ask what happened.
       out.skippedForeign += 1;
-      logger?.warn?.('[security] permission set name owned by another package — skipped', {
-        name: ps.name, declaredBy: packageId, ownedBy: existing.package_id,
+      const diagnostic = permissionSetNameCollisionDiagnostic({
+        name: String(ps.name),
+        declaredBy: packageId,
+        ownedBy: typeof existing.package_id === 'string' ? existing.package_id : null,
+        ...(organizationId ? { organizationId } : {}),
       });
+      out.collisions = [diagnostic];
+      // The boot loop collects and reports ONCE per pass. The ADR-0086 P2
+      // publish materializer upserts a single set and passes no collector, so
+      // it reports here — it has no pass to summarise, and inheriting the old
+      // silence is the one outcome this card forbids.
+      if (opts?.collisions) opts.collisions.push(diagnostic);
+      else reportPermissionSetNameCollisions(logger, [diagnostic], organizationId);
     }
     return out;
   }
@@ -344,13 +385,15 @@ export async function bootstrapDeclaredPermissions(
   // One log per pass, not per refused row: a legacy platform-wide unique index
   // refuses EVERY declared permission set, and a line each would bury the remedy.
   const refusals = createSeedWriteRefusals();
+  // [#17516] Declared sets dropped because another package owns the name.
+  const collisions: PermissionSetNameCollisionDiagnostic[] = [];
 
   for (const ps of sets) {
     if (!ps?.name) continue;
     // Registry provenance first (ADR-0010 `_packageId`), author-declared
     // spec `packageId` (ADR-0086 D3) as fallback.
     const packageId: string | undefined = ps._packageId ?? ps.packageId ?? undefined;
-    const r = await upsertPackagePermissionSet(ql, ps, packageId, options.logger, { existingByName, organizationId, residue, refusals });
+    const r = await upsertPackagePermissionSet(ql, ps, packageId, options.logger, { existingByName, organizationId, residue, refusals, collisions });
     out.seeded += r.seeded;
     out.updated += r.updated;
     out.unchanged += r.unchanged;
@@ -370,6 +413,11 @@ export async function bootstrapDeclaredPermissions(
   }
   // Before the counts, so an operator reads WHY the count is zero beside it.
   reportSeedWriteRefusals(options.logger, refusals, organizationId);
+  // [#17516] Said once per pass, and said even when no logger was injected —
+  // the whole defect was that this refusal reached nobody. The records go back
+  // on the outcome too, for a caller that reads no log at all.
+  reportPermissionSetNameCollisions(options.logger, collisions, organizationId);
+  if (collisions.length > 0) out.collisions = collisions;
   if (out.unreadable > 0) {
     // Said once, with the count: these sets were neither seeded nor reconciled
     // because the record could not be READ. Silence here would read exactly

packages/plugins/plugin-security/src/bootstrap-seed-round-trips.test.ts

@@ -431,7 +431,14 @@ describe('#10946 — a name declared twice in one batch keeps its loud refusal',
     expect(r.skippedForeign).toBe(1);
     expect(ql.rows).toHaveLength(1);
     expect(ql.rows[0].package_id).toBe('com.example.a');
-    expect(warns.some((w) => w.includes('owned by another package'))).toBe(true);
+    // [#17516] Re-anchored from the old prose to the stable token the report
+    // stamps — the assertion's substance (the refusal is REPORTED, not merely
+    // counted) is unchanged, and the record is asserted beside it so "loud"
+    // means reaching a reader rather than moving a counter.
+    expect(warns.some((w) => w.includes('permission_set_name_collision'))).toBe(true);
+    expect(r.collisions).toEqual([
+      expect.objectContaining({ name: 'shared_name', declaredBy: 'com.example.b', ownedBy: 'com.example.a' }),
+    ]);
   });
 });
 

packages/plugins/plugin-security/src/index.ts

@@ -55,6 +55,23 @@ export type {
   InvitationPlacementService,
 } from './invitation-placement.js';
 export { bootstrapDeclaredPermissions } from './bootstrap-declared-permissions.js';
+// [#17516] The set-name collision diagnostic. EXPORTED because its whole
+// purpose is to be the ONE derivation every door shares: the runtime door below
+// raises it today, and the compile-time door (`os build` / `os validate`, which
+// lives in another package) must consume these rather than re-deriving either
+// the predicate or the wording — that drift is what this card is about, one
+// layer up.
+export {
+  PERMISSION_SET_NAME_COLLISION,
+  formatPermissionSetNameCollisionDiagnostic,
+  permissionSetNameCollisionDiagnostic,
+  permissionSetNameIsForeign,
+  reportPermissionSetNameCollisions,
+} from './permission-set-name-collision.js';
+export type {
+  CollisionReportSink,
+  PermissionSetNameCollisionDiagnostic,
+} from './permission-set-name-collision.js';
 // [ADR-0094] sys_permission_set pure-projection machinery.
 export {
   permissionSetRowFields,