From e2a3b1110ca90176e4d208604ed1465a6c7a768d Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 11:38:40 +0000 Subject: [PATCH 1/4] fix(plugin-security): make a permission-set name collision reach the author MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A package's entire declared permission set was dropped on a set-name collision with no author-visible output at all: the branch reported through `logger?.warn?.(...)`, optionally chained twice, so a caller passing no logger produced nothing and the set vanished with one counter moved. The comment there said "refuse loudly"; nothing about it was loud. The skip itself is unchanged and correct (ADR-0086 D4: a package never writes into a foreign record). What changes is that the refusal is now reported — through the console when no sink was injected, per the #10556 ruling that silent-by-declaration is rejected — and that the diagnostic record travels back on the pass outcome, so a caller reading no log can still ask which sets were dropped and why. The collision predicate, the wording and the formatter live in one module so the compile-time door can consume them rather than re-derive them, which is the shape #14553 established for `navigationContributions`. Only the runtime door is built here; the compile-time door lives in another package. Also corrects the branch comment's premise. 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. Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj Co-authored-by: Claude --- .../bootstrap-declared-permissions.test.ts | 11 +- .../src/bootstrap-declared-permissions.ts | 62 +++- packages/plugins/plugin-security/src/index.ts | 17 + .../src/permission-set-name-collision.test.ts | 315 ++++++++++++++++++ .../src/permission-set-name-collision.ts | 224 +++++++++++++ .../src/permission-set-projection.ts | 13 + 6 files changed, 634 insertions(+), 8 deletions(-) create mode 100644 packages/plugins/plugin-security/src/permission-set-name-collision.test.ts create mode 100644 packages/plugins/plugin-security/src/permission-set-name-collision.ts diff --git a/packages/plugins/plugin-security/src/bootstrap-declared-permissions.test.ts b/packages/plugins/plugin-security/src/bootstrap-declared-permissions.test.ts index 2cfa72f197..97bd99c6f9 100644 --- a/packages/plugins/plugin-security/src/bootstrap-declared-permissions.test.ts +++ b/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 () => { diff --git a/packages/plugins/plugin-security/src/bootstrap-declared-permissions.ts b/packages/plugins/plugin-security/src/bootstrap-declared-permissions.ts index b83f836070..c11b5d82fb 100644 --- a/packages/plugins/plugin-security/src/bootstrap-declared-permissions.ts +++ b/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 { 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 diff --git a/packages/plugins/plugin-security/src/index.ts b/packages/plugins/plugin-security/src/index.ts index a80726af87..228fe0e20e 100644 --- a/packages/plugins/plugin-security/src/index.ts +++ b/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, diff --git a/packages/plugins/plugin-security/src/permission-set-name-collision.test.ts b/packages/plugins/plugin-security/src/permission-set-name-collision.test.ts new file mode 100644 index 0000000000..a41cbdf6a1 --- /dev/null +++ b/packages/plugins/plugin-security/src/permission-set-name-collision.test.ts @@ -0,0 +1,315 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#17516] The set-name collision 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_permission_set` 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 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 permission set disappeared with one + * internal counter moved. + * + * ## The measurement these replace + * + * On the pre-fix tree 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 set, which would be 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 and really does + * re-seed its own row. + */ + +import { describe, it, expect, vi } from 'vitest'; + +import { + bootstrapDeclaredPermissions, + upsertPackagePermissionSet, +} from './bootstrap-declared-permissions.js'; +import { + PERMISSION_SET_NAME_COLLISION, + formatPermissionSetNameCollisionDiagnostic, + permissionSetNameCollisionDiagnostic, + permissionSetNameIsForeign, + reportPermissionSetNameCollisions, +} from './permission-set-name-collision.js'; + +/** Minimal in-memory ql + registry for sys_permission_set seeding. */ +function makeQl(declared: any[] = []) { + const rows: any[] = []; + return { + rows, + registry: { listItems: (type: string) => (type === 'permission' ? declared : []) }, + async find(object: string, q: any) { + if (object !== 'sys_permission_set') return []; + const where = q?.where ?? {}; + return rows.filter((r) => Object.entries(where).every(([k, v]) => { + if (v && typeof v === 'object' && !Array.isArray(v)) { + const inList = (v as any).$in; + if (Array.isArray(inList)) return inList.includes(r[k]); + throw new Error(`fake driver: unsupported operator ${Object.keys(v).join(',')}`); + } + return r[k] === v; + })); + }, + async insert(object: string, data: any) { + if (object !== 'sys_permission_set') return null; + rows.push({ ...data }); + return { id: data.id }; + }, + async update(object: string, data: any) { + if (object !== 'sys_permission_set') return; + const r = rows.find((x) => x.id === data.id); + if (r) Object.assign(r, data); + }, + }; +} + +const declaredSet = (over: Record = {}) => ({ + name: 'crm_sales_rep', + label: 'Sales Rep', + objects: { crm_lead: { allowRead: true } }, + _packageId: 'com.example.b', + ...over, +}); + +/** + * Capture EVERY console channel. "Author-visible output" is not channel-specific + * — the pre-fix failure was that NONE of them carried anything — so a pin that + * watched only `warn` could be satisfied by a change that moved the silence. + */ +function captureConsole() { + 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('[#17516] set-name collision reaches the author', () => { + it('prints, WITH NO LOGGER INJECTED, and returns the diagnostic record', async () => { + const ql = makeQl([declaredSet()]); + ql.rows.push({ + id: 'ps_owned_by_a', + name: 'crm_sales_rep', + managed_by: 'package', + package_id: 'com.example.a', + object_permissions: '{}', + }); + + const cap = captureConsole(); + let out: Awaited>; + try { + // No logger — the exact call shape under which the old branch was mute. + out = await bootstrapDeclaredPermissions(ql, undefined); + } finally { + cap.restore(); + } + + // ⛔ The refusal itself is UNCHANGED. + expect(out.skippedForeign).toBe(1); + expect(out.seeded).toBe(0); + expect(out.updated).toBe(0); + expect(ql.rows).toHaveLength(1); + expect(ql.rows[0].package_id).toBe('com.example.a'); + expect(ql.rows[0].object_permissions).toBe('{}'); + + // The half that was missing: the author is told, through the console, + // because no sink was injected (#10556 — silent-by-declaration rejected). + expect(cap.seen).toHaveLength(1); + expect(cap.seen[0]).toContain(PERMISSION_SET_NAME_COLLISION); + expect(cap.seen[0]).toMatch(/^warn:/); + expect(cap.seen[0]).toContain('NOT materialized'); + + // And the RECORD, for a caller that reads no log at all. + expect(out.collisions).toHaveLength(1); + const d = out.collisions![0]!; + expect(d.event).toBe(PERMISSION_SET_NAME_COLLISION); + expect(d.severity).toBe('warning'); + expect(d.name).toBe('crm_sales_rep'); + expect(d.declaredBy).toBe('com.example.b'); + expect(d.ownedBy).toBe('com.example.a'); + // The remedy names both legal resolutions, including the ADR-0130 D1 one + // the old comment's premise denied could exist. + expect(d.fix).toContain('ADR-0130 D1'); + }); + + it('DISCRIMINATING CONTROL: a pass with no collision says nothing at all', async () => { + // Two real writes happen here — a first seed and an own-row re-seed — so + // the silence is over a pass that is doing work, not an empty one. + const ql = makeQl([declaredSet({ _packageId: 'com.example.a' })]); + + const first = captureConsole(); + let r1: Awaited>; + try { + r1 = await bootstrapDeclaredPermissions(ql, undefined); + } finally { + first.restore(); + } + expect(r1.seeded).toBe(1); + expect(r1.skippedForeign).toBe(0); + expect(r1.collisions).toBeUndefined(); + expect(first.seen).toEqual([]); + + // Re-seed the SAME package's own row after a declaration change. + (ql as any).registry = { + listItems: () => [declaredSet({ _packageId: 'com.example.a', objects: { crm_lead: { allowRead: true, allowCreate: true } } })], + }; + const second = captureConsole(); + let r2: Awaited>; + try { + r2 = await bootstrapDeclaredPermissions(ql, undefined); + } finally { + second.restore(); + } + expect(r2.updated).toBe(1); + expect(r2.skippedForeign).toBe(0); + expect(r2.collisions).toBeUndefined(); + expect(second.seen).toEqual([]); + }); + + it('reports through an INJECTED sink instead of the console when one is given', async () => { + const ql = makeQl([declaredSet()]); + ql.rows.push({ + id: 'ps_owned_by_a', + name: 'crm_sales_rep', + managed_by: 'package', + package_id: 'com.example.a', + object_permissions: '{}', + }); + + const warned: Array<{ m: string; meta?: Record }> = []; + const cap = captureConsole(); + try { + await bootstrapDeclaredPermissions(ql, undefined, { + logger: { warn: (m, meta) => { warned.push({ m, meta }); } }, + }); + } finally { + cap.restore(); + } + + const collision = warned.filter((w) => w.m.includes(PERMISSION_SET_NAME_COLLISION)); + expect(collision).toHaveLength(1); + expect(collision[0]!.meta?.collisions).toEqual([ + expect.objectContaining({ name: 'crm_sales_rep', declaredBy: 'com.example.b', ownedBy: 'com.example.a' }), + ]); + // The host sink REPLACES the console default — not both. + expect(cap.seen).toEqual([]); + }); + + it('the ADR-0086 P2 single-set path reports too — it has no pass to summarise', async () => { + const ql = makeQl(); + ql.rows.push({ + id: 'ps_owned_by_a', + name: 'crm_sales_rep', + managed_by: 'package', + package_id: 'com.example.a', + object_permissions: '{}', + }); + + const cap = captureConsole(); + let out: Awaited>; + try { + // No logger, no collector — the publish materializer's exact call shape. + out = await upsertPackagePermissionSet(ql, declaredSet(), 'com.example.b'); + } finally { + cap.restore(); + } + + expect(out.skippedForeign).toBe(1); + expect(out.collisions).toHaveLength(1); + expect(cap.seen).toHaveLength(1); + expect(cap.seen[0]).toContain(PERMISSION_SET_NAME_COLLISION); + }); + + it('a package-managed row with NO package_id is foreign, not adoptable', async () => { + const ql = makeQl([declaredSet()]); + // ADR-0086 D3's exact ambiguity: managed_by 'package', owner unprovable. + ql.rows.push({ + id: 'ps_unowned', + name: 'crm_sales_rep', + managed_by: 'package', + object_permissions: '{}', + }); + + const cap = captureConsole(); + let out: Awaited>; + try { + out = await bootstrapDeclaredPermissions(ql, undefined); + } finally { + cap.restore(); + } + + expect(out.skippedForeign).toBe(1); + expect(out.updated).toBe(0); + expect(ql.rows[0].object_permissions).toBe('{}'); + expect(out.collisions![0]!.ownedBy).toBeNull(); + expect(cap.seen).toHaveLength(1); + }); +}); + +describe('[#17516] the shared derivation both doors must use', () => { + it('permissionSetNameIsForeign: same owner is ours, anything else is foreign', () => { + expect(permissionSetNameIsForeign('com.example.a', 'com.example.a')).toBe(false); + expect(permissionSetNameIsForeign('com.example.a', 'com.example.b')).toBe(true); + // A nullish owner is FOREIGN — never silently adopted on a name match. + expect(permissionSetNameIsForeign(undefined, 'com.example.b')).toBe(true); + expect(permissionSetNameIsForeign(null, 'com.example.b')).toBe(true); + // Both unowned is not a collision between two different packages. + expect(permissionSetNameIsForeign(null, undefined)).toBe(false); + }); + + /** + * The literal is asserted rather than only imported — the precedent the + * sibling `position_name_fold_grant` pin sets. Importing it on both sides + * would let a rename pass green while every operator's grep went dead. + */ + it('stamps a stable, greppable token', () => { + expect(PERMISSION_SET_NAME_COLLISION).toBe('permission_set_name_collision'); + const d = permissionSetNameCollisionDiagnostic({ + name: 'crm_sales_rep', declaredBy: 'com.example.b', ownedBy: 'com.example.a', + }); + expect(formatPermissionSetNameCollisionDiagnostic(d)) + .toContain('[security] [permission_set_name_collision]'); + }); + + it('reportPermissionSetNameCollisions says nothing when there is nothing to say', () => { + const cap = captureConsole(); + try { + reportPermissionSetNameCollisions(undefined, []); + } finally { + cap.restore(); + } + expect(cap.seen).toEqual([]); + }); + + /** + * The measured-and-rejected spelling next door: `(logger.warn ?? console.warn)(…)` + * detaches the receiver, and a class-based host sink reaching for `this` + * throws. Every double in this package is a plain closure and would survive + * it, so the pin uses a real class. + */ + it('keeps the receiver when the host sink is a class instance', () => { + class HostSink { + lines: string[] = []; + warn(m: string): void { this.lines.push(m); } + } + const sink = new HostSink(); + const d = permissionSetNameCollisionDiagnostic({ + name: 'crm_sales_rep', declaredBy: 'com.example.b', ownedBy: 'com.example.a', + }); + expect(() => reportPermissionSetNameCollisions(sink, [d])).not.toThrow(); + expect(sink.lines).toHaveLength(1); + }); +}); diff --git a/packages/plugins/plugin-security/src/permission-set-name-collision.ts b/packages/plugins/plugin-security/src/permission-set-name-collision.ts new file mode 100644 index 0000000000..5aa5ff77e8 --- /dev/null +++ b/packages/plugins/plugin-security/src/permission-set-name-collision.ts @@ -0,0 +1,224 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#17516] The set-name collision diagnostic — ONE derivation, ONE wording, for + * every door that has to tell an author their declared permission set was not + * materialized. + * + * ## What was wrong + * + * `bootstrapDeclaredPermissions` refuses to write into a `sys_permission_set` + * row another package owns (ADR-0086 D4: a package never writes into a foreign + * record). That refusal is CORRECT and is unchanged here. What was wrong is + * that it was INVISIBLE: the whole declared set vanished with an internal + * counter incremented and one `logger?.warn?.(…)` line — optionally chained + * TWICE, so a caller that passed no logger produced no output at all. The + * comment at that branch said "refuse loudly"; nothing about it was loud, and + * nothing was written back anywhere an author could read it. + * + * That is the #10556 doctrine's own failure shape, one surface over: a report + * channel that is silent by declaration. The maintainer ruling there + * (2026-08-24) rejected silent-by-declaration and made `SecurityPlugin`'s own + * sink console-backed by default — loud until a host injects one. + * {@link reportPermissionSetNameCollisions} carries the same guarantee for this + * refusal, which is why it takes a possibly-absent sink and still prints. + * + * ## Why a module, and not two call sites + * + * The precedent this card names is #14553's `navigationContributions` fix: a + * diagnostic raised at BOTH doors — runtime and compile (`os build` / + * `os validate`) — behind ONE shared predicate so the two cannot drift. A + * compile-time door that called a collision fine while the runtime dropped the + * set would be indistinguishable, to the author, from the silence being fixed + * here. + * + * ⚠️ Only the RUNTIME door is built here. The compile-time door lives in + * `packages/cli` (`domain:cli`), a different lane, and this change deliberately + * does not reach into it. What this module exists to guarantee is that when + * that door is built it consumes {@link permissionSetNameIsForeign} and + * {@link permissionSetNameCollisionDiagnostic} rather than re-deriving either — + * which is why both, and the wording, are exported from the package entry + * rather than kept module-private. + * + * ## Why `event` and not `code` + * + * {@link PERMISSION_SET_NAME_COLLISION} is a snake_case DATA VALUE, not an + * ADR-0112 error code: it is never routed to `error.code`, never reaches a + * wire refusal, and travels as the structured half of a warning about an + * artifact. That is the discrimination the sibling `position_name_fold_grant` + * and `platform_owner_wall_bypass` tokens in `security-plugin.ts` already make + * in this package, and it is why the stamp below is spelled `event:` rather + * than `code:` — ⛔ a `code:` position here would be claiming a vocabulary this + * value is not in. + */ + +/** + * The minimal sink this module reports through. + * + * Structurally satisfied by both `ProjectionLogger` and the catalog's + * `SeedLogger` — declared here rather than imported so this module stays a leaf + * and `permission-set-projection.ts` can name the diagnostic type without a + * cycle. `warn` is non-optional for the #9754 reason its two siblings give: a + * fallback channel that may itself be absent is not a fallback. + */ +export interface CollisionReportSink { + warn: (message: string, meta?: Record) => void; +} + +/** + * The stable token stamped on every set-name-collision report, so an operator + * can grep ONE string for every silently-dropped permission set. + * + * Exported — unlike the sibling `position_name_fold_grant`, which is + * deliberately package-private because nothing outside ever consumes it. This + * one has a second consumer by construction: the compile-time door this card's + * precedent requires. A door that re-spells the literal is the drift the shared + * derivation exists to prevent. + */ +export const PERMISSION_SET_NAME_COLLISION = 'permission_set_name_collision'; + +/** One declared permission set that was NOT materialized because another package owns its name. */ +export interface PermissionSetNameCollisionDiagnostic { + /** Always {@link PERMISSION_SET_NAME_COLLISION}. */ + readonly event: typeof PERMISSION_SET_NAME_COLLISION; + /** + * Never `error`: the platform refuses the write and carries on, and the + * failure direction is CLOSED — the set is not installed, so nothing is + * over-granted. ⚠️ Were a path ever measured in which this drop ALLOWS an + * access that should have been refused, that is a different severity and a + * different card. + */ + readonly severity: 'warning'; + /** The declared set's name — the `sys_permission_set.name` that collided. */ + readonly name: string; + /** The package whose declaration was dropped. */ + readonly declaredBy: string; + /** + * The package that owns the standing row. `null` when the row is + * package-managed but carries no `package_id` — an unowned row is still not + * ours to write, and saying so is more useful than printing `undefined`. + */ + readonly ownedBy: string | null; + /** The organization whose catalog pass hit this, when the pass was scoped. */ + readonly organizationId?: string; + readonly message: string; + readonly fix: string; +} + +/** + * THE shared predicate: is the standing row's owner a DIFFERENT package from + * the one declaring this set? + * + * Both doors ask exactly this question and must get exactly this answer. + * + * ⚠️ Deliberately asks only about OWNERS, because that is the only datum the + * two doors certainly share: the runtime door reads `package_id` off a + * `sys_permission_set` row, while a compile-time door composing one artifact + * reads it off whichever declaration claimed the name first. Folding the row's + * `managed_by` check in here would make the predicate unanswerable from a + * declaration — the caller decides that a row is package-managed at all + * (env-authored rows are a different branch entirely and are never clobbered), + * and then asks this. + * + * ⚠️ A nullish owner is FOREIGN, not "ours". A package-managed row with no + * `package_id` is the exact ambiguity ADR-0086 D3 exists to remove, and + * adopting it on a name match would be a package writing into a record it + * cannot prove it owns. This preserves the behaviour the branch already had — + * ⛔ this card changes what the author is TOLD, never what is skipped. + */ +export function permissionSetNameIsForeign( + ownerPackageId: string | null | undefined, + declaringPackageId: string | null | undefined, +): boolean { + return (ownerPackageId ?? null) !== (declaringPackageId ?? null); +} + +/** + * Word ONE collision. Callers decide reachability (see the seeder's + * package-managed branch); this only words the finding, so both doors print + * the same sentence. + */ +export function permissionSetNameCollisionDiagnostic(input: { + name: string; + declaredBy: string; + ownedBy?: string | null; + organizationId?: string; +}): PermissionSetNameCollisionDiagnostic { + const ownedBy = input.ownedBy ?? null; + const owner = ownedBy ?? '(a package-managed row with no package_id)'; + return { + event: PERMISSION_SET_NAME_COLLISION, + severity: 'warning', + name: input.name, + declaredBy: input.declaredBy, + ownedBy, + ...(input.organizationId === undefined ? {} : { organizationId: input.organizationId }), + message: + `Package "${input.declaredBy}" declares permission set "${input.name}", but that set name is ` + + `already owned by ${owner}. The ENTIRE declared set was NOT materialized — a package never ` + + `writes into a foreign record (ADR-0086 D4) — so none of its object, field, tab or system ` + + `permissions are in effect, and nothing else reports this.`, + fix: + `Rename the set in package "${input.declaredBy}" to a name it owns (prefix it with the ` + + `package's own module prefix), or — if the two packages genuinely co-own this namespace ` + + `(ADR-0130 D1) — have exactly one of them declare the set and let the other depend on it.`, + }; +} + +/** One line carrying the whole finding — the text every door prints. */ +export function formatPermissionSetNameCollisionDiagnostic( + d: PermissionSetNameCollisionDiagnostic, +): string { + return `[security] [${d.event}] ${d.message} Fix: ${d.fix}`; +} + +/** + * Report every collision one catalog pass found — the half that makes the + * refusal reach the author. + * + * ⛔ NOT `logger?.warn?.(…)`. That is the defect this module exists to remove: + * with no sink injected it evaluates to nothing at all, and the set disappears + * with only a counter moved. A caller that passes no sink gets `console.warn`, + * for the reason #10556's ruling gives — loud until a host injects one. + * + * ⛔ NOT `(logger?.warn ?? console.warn)(…)` either: that evaluates to a bare + * function and calls it with `this === undefined`, and a class-based host sink + * (`@objectstack/core`'s `ObjectLogger`) reaches for `this` and throws. The + * property-access call form below keeps the receiver — the same measured + * conclusion `logSeedDurabilityFailure` records next door. + * + * ONE line per pass, not one per dropped set: a package that collides on its + * whole declaration would otherwise bury its own remedy. Every diagnostic + * travels in the structured meta beside it, and the caller gets the records + * themselves on the pass outcome, so nothing is summarised away. + * + * `warn`, not `error`: this is a FUNCTIONAL degradation in the AGENTS.md sense + * — the deployment is visibly smaller than it was authored to be, and the next + * principal who needs the grant is refused. Nothing claimed to be persisted + * silently failed to land; the write was deliberately never attempted. + */ +export function reportPermissionSetNameCollisions( + logger: CollisionReportSink | undefined, + collisions: readonly PermissionSetNameCollisionDiagnostic[], + organizationId?: string, +): void { + if (collisions.length === 0) return; + const n = collisions.length; + const message = + `[security] [${PERMISSION_SET_NAME_COLLISION}] ${n} declared permission ` + + `set${n === 1 ? ' was' : 's were'} NOT materialized — another package already owns ` + + `${n === 1 ? 'that set name' : 'those set names'} (ADR-0086 D4). ` + + `The declaring package's permissions are NOT in effect.`; + const meta = { + event: PERMISSION_SET_NAME_COLLISION, + ...(organizationId ? { organization: organizationId } : {}), + collisions: collisions.map((d) => ({ + name: d.name, + declaredBy: d.declaredBy, + ownedBy: d.ownedBy, + fix: d.fix, + })), + }; + if (logger) logger.warn(message, meta); + else console.warn(message, meta); +} diff --git a/packages/plugins/plugin-security/src/permission-set-projection.ts b/packages/plugins/plugin-security/src/permission-set-projection.ts index f8701b7b42..341778fad5 100644 --- a/packages/plugins/plugin-security/src/permission-set-projection.ts +++ b/packages/plugins/plugin-security/src/permission-set-projection.ts @@ -93,6 +93,7 @@ import { PermissionSetSchema } from '@objectstack/spec/security'; import { seedCtx, type SeedWriteRefusals } from './per-organization-catalog.js'; import { buildExistingByName, type ExistingByNameIndex } from './seed-name-lookup.js'; +import type { PermissionSetNameCollisionDiagnostic } from './permission-set-name-collision.js'; import { ENV_PROJECTION_MARKER, assertPermissionSetNotPackageDeclared, @@ -219,6 +220,18 @@ export interface PermissionSeedOutcome { skippedForeign: number; /** Records retired because their definition was deleted from metadata. */ deleted?: number; + /** + * [#17516] The diagnostic for each set counted in {@link skippedForeign} — + * the declaration was dropped WHOLE because another package owns that set + * name (ADR-0086 D4). + * + * ⚠️ This is the read-back half of that refusal, not a duplicate of the log + * line. The counter alone is what made the drop invisible: a caller holding + * it knows a number and nothing about which sets, which packages, or what to + * do about it. Absent — never `[]` — when the pass hit no collision, so a + * consumer can tell "none" apart from "this pass does not report them". + */ + collisions?: readonly PermissionSetNameCollisionDiagnostic[]; } /** From e9b8ae40befa2930aa654bcdd6b55268feec1928 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 11:50:18 +0000 Subject: [PATCH 2/4] test(plugin-security): re-anchor collision pins to the stable token, add changeset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two existing pins asserted the old prose ('owned by another package'). Their substance — the refusal is REPORTED, not merely counted — is unchanged; they now assert the stable `permission_set_name_collision` token an operator greps and the sibling doors key on, plus the diagnostic record beside it, so prose drift cannot quietly unpin them. Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj Co-authored-by: Claude --- ...516-permission-set-collision-diagnostic.md | 29 +++++++++++++++++++ .../src/bootstrap-seed-round-trips.test.ts | 9 +++++- 2 files changed, 37 insertions(+), 1 deletion(-) create mode 100644 .changeset/17516-permission-set-collision-diagnostic.md diff --git a/.changeset/17516-permission-set-collision-diagnostic.md b/.changeset/17516-permission-set-collision-diagnostic.md new file mode 100644 index 0000000000..debf72fa9c --- /dev/null +++ b/.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. diff --git a/packages/plugins/plugin-security/src/bootstrap-seed-round-trips.test.ts b/packages/plugins/plugin-security/src/bootstrap-seed-round-trips.test.ts index a3c42c4132..74802f50d3 100644 --- a/packages/plugins/plugin-security/src/bootstrap-seed-round-trips.test.ts +++ b/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' }), + ]); }); }); From 6a5958acd198a14d5ceed3d65ef31f643d5f3847 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 12:19:59 +0000 Subject: [PATCH 3/4] test(plugin-security): pin the new engine double through assertEngineUpdateDispatch `check:engine-double-contract` named the new fake's `update()` as a double looser than `ObjectQL.update`. It now opens with `assertEngineUpdateDispatch` and is recorded in the pinned ledger, regenerated with the repo's own `--write` (799 rows, 1 added or grown, 0 lost; byte-identical across two runs). Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj Co-authored-by: Claude --- .../src/permission-set-name-collision.test.ts | 10 ++++++++-- scripts/engine-double-contract.pinned.json | 5 +++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/packages/plugins/plugin-security/src/permission-set-name-collision.test.ts b/packages/plugins/plugin-security/src/permission-set-name-collision.test.ts index a41cbdf6a1..aa5aff2891 100644 --- a/packages/plugins/plugin-security/src/permission-set-name-collision.test.ts +++ b/packages/plugins/plugin-security/src/permission-set-name-collision.test.ts @@ -30,6 +30,7 @@ */ import { describe, it, expect, vi } from 'vitest'; +import { assertEngineUpdateDispatch } from '@objectstack/metadata-core'; import { bootstrapDeclaredPermissions, @@ -66,9 +67,14 @@ function makeQl(declared: any[] = []) { rows.push({ ...data }); return { id: data.id }; }, - async update(object: string, data: any) { + // `assertEngineUpdateDispatch` rather than a hand-rolled id check: a fake + // looser than `ObjectQL.update` is how a dead route once shipped with its + // suite green (`check:engine-double-contract`). + async update(object: string, data: any, options?: any) { + const dispatch = assertEngineUpdateDispatch(data, options); if (object !== 'sys_permission_set') return; - const r = rows.find((x) => x.id === data.id); + if (dispatch.kind !== 'by-id') throw new Error('fake driver: only by-id update is modelled'); + const r = rows.find((x) => x.id === dispatch.id); if (r) Object.assign(r, data); }, }; diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index 68b8a906d9..e0654a41d4 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -2886,6 +2886,11 @@ "verb": "update", "pinned": 1 }, + { + "file": "packages/plugins/plugin-security/src/permission-set-name-collision.test.ts", + "verb": "update", + "pinned": 1 + }, { "file": "packages/plugins/plugin-security/src/permission-set-overlay-discard.test.ts", "verb": "delete", From 498f6054345cba3f530f991d7d4781bb5d2d45c5 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 13 Sep 2026 13:58:40 +0000 Subject: [PATCH 4/4] test(plugin-security): the collision fake refuses `$or` and holds the caller's bound MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two repo gates graded the `find` double this branch adds, and both reds are this branch's own. CI stops at the first non-zero exit, so only the first was visible: the second sat in the unmeasured tail behind it. 1. `check:where-matcher` -- SILENTLY WRONG (shape (b)). A top-level `$or` / `$and` carries an ARRAY value, so the operator-object arm never sees it and the key falls through to `r[k] === v`. `r.$or` is `undefined`, no row matches, and a suite asserts on an empty result set with nothing erroring. Measured on the real bytes, lifted out of the file and handed `{ name: 'crm_sales_rep', $or: [...] }` where the seeded row satisfies the name AND the first arm: before RETURNED [] <- the row a real driver returns, silently dropped after THREW Error: fake driver: unsupported combinator $or 2. `check:objectql-double-limit` -- BLIND to the caller's bound. `defaultLookup` really does read `{ where: { name }, limit: organizationId ? 5 : 1 }`, and the bound is load-bearing (#10103: a scoped read returns organization-less rows beside this organization's own). A limit-blind double cannot tell that read from an unbounded one, so every change to that bound was green by construction. Both remedies are the ones the gate headers prescribe, and both spellings are the sibling double's in this same package (`objects/reserved-identity-names.test.ts`): one `throw` rather than a combinator implementation -- "the defect class is *silence*, not incompleteness" -- and the bound applied by PRESENCE (`typeof q?.limit === 'number'`, so `limit: 0` returns nothing rather than everything) AFTER the filter. ⛔ Neither baseline grows, and neither gate is weakened, skipped or re-ordered: where-matcher 408 discovered, 408 conforming (270 refuse, +1), no files added objectql-double-limit 386 graded, 188 apply the bound or refuse (+1), no files added Claude-Session: https://claude.ai/code/session_01URLHobLUJB9K1ABV6ofdjj Co-authored-by: Claude --- .../src/permission-set-name-collision.test.ts | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/packages/plugins/plugin-security/src/permission-set-name-collision.test.ts b/packages/plugins/plugin-security/src/permission-set-name-collision.test.ts index aa5aff2891..7e73aa0b7e 100644 --- a/packages/plugins/plugin-security/src/permission-set-name-collision.test.ts +++ b/packages/plugins/plugin-security/src/permission-set-name-collision.test.ts @@ -53,7 +53,13 @@ function makeQl(declared: any[] = []) { async find(object: string, q: any) { if (object !== 'sys_permission_set') return []; const where = q?.where ?? {}; - return rows.filter((r) => Object.entries(where).every(([k, v]) => { + const hits = rows.filter((r) => Object.entries(where).every(([k, v]) => { + // ⛔ REFUSE what this double does not implement. A `$or` / `$and` read + // as a FIELD NAME is the silently-wrong shape: `r.$or` is `undefined`, + // no row matches, and a suite asserts on an empty result set with + // nothing erroring (`check:where-matcher`, shape (b)). The sibling + // double in `objects/reserved-identity-names.test.ts` refuses the same way. + if (k.startsWith('$')) throw new Error(`fake driver: unsupported combinator ${k}`); if (v && typeof v === 'object' && !Array.isArray(v)) { const inList = (v as any).$in; if (Array.isArray(inList)) return inList.includes(r[k]); @@ -61,6 +67,12 @@ function makeQl(declared: any[] = []) { } return r[k] === v; })); + // Hold the caller's BOUND, after the filter and by PRESENCE, so `limit: 0` + // returns nothing rather than everything. `defaultLookup` really does read + // `{ where: { name }, limit: organizationId ? 5 : 1 }` (#10103), and a + // limit-blind double cannot tell that read from an unbounded one + // (`check:objectql-double-limit`). + return typeof q?.limit === 'number' ? hits.slice(0, q.limit) : hits; }, async insert(object: string, data: any) { if (object !== 'sys_permission_set') return null;