fix(plugin-security): report the two swallowed tryUpdate refusals outside the catalog seed

.changeset/permission-set-refusal-visibility.md

@@ -0,0 +1,44 @@
+---
+"@objectstack/plugin-security": patch
+---
+
+fix(plugin-security): report the two swallowed `tryUpdate` refusals outside the catalog seed (#12970)
+
+Both sites call the shared `tryUpdate` in `permission-set-projection.ts`, which
+answers `false` on refusal. That answer is byte-identical to "nothing to do",
+and neither caller passed the optional refusal log the helper already accepts —
+so a refused write was indistinguishable from a clean pass.
+
+**`permission-set-drift.ts` — a refused diagnostic write silenced its own
+report.** `persistPermissionSetDriftDiagnostics` counted only the writes that
+landed, and `runPermissionSetDriftDiagnostics` reported only when that count was
+non-zero. A boot on which every drift write was refused computed the drift
+correctly, persisted none of it, and printed nothing at all — indistinguishable
+from a deployment with no drift, while the sets kept enforcing grants that
+differ from the shipped artifact. The pass now records refusals, answers a
+`refused` count beside `updated`, reports them once per pass on the durability
+channel, and emits the drifted-set line when writes were refused as well as when
+they landed. A steady-state boot (nothing to write, nothing refused) stays
+exactly as quiet as before.
+
+**`permission-set-overlay-discard.ts` — the audit line could describe a discard
+that did not happen.** On the degraded-kernel branch the resync write's result
+was discarded entirely. On refusal the row was re-read unchanged, so
+`objectGrantsAfter` equalled `objectGrantsBefore` while the `info` entry still
+announced a completed "sanctioned operator action": every field individually
+true, the entry as a whole false. The result is now read, and a refused resync
+emits one entry stating what did and did not land — the overlay row deletion
+(which had already succeeded) and the refused resync, with the un-healed grant
+count named as such — **instead of** the success line, never alongside it.
+
+Both new lines go through the shared durability channel with its mandatory
+`warn` fallback, so they still print against a host sink that has no `error`.
+They reuse the shared refusal *accumulator* (`createSeedWriteRefusals`, with its
+cross-dialect classification and value-free driver-code channel) but not
+`reportSeedWriteRefusals`, whose prose is specific to seeding the RBAC catalog
+and would misdiagnose either of these paths.
+
+No API is removed or narrowed. `persistPermissionSetDriftDiagnostics` and
+`runPermissionSetDriftDiagnostics` answer one additional field (`refused`), and
+what `discardPermissionSetOverlay` returns to its caller is deliberately
+unchanged.

packages/plugins/plugin-security/src/per-organization-catalog.ts

@@ -145,8 +145,18 @@ export type SeedLogger = {
  *
  * The `?.` on `warn` is the backstop for hosts the TYPE cannot reach (a
  * plain-JS embedder, or a cast), not doubt about the declaration.
+ *
+ * EXPORTED rather than module-private, for the reason its own doc gives — the
+ * fallback "lives in {@link logSeedDurabilityFailure} so no site can forget
+ * it". Two sites outside the catalog seed now report a refused write and owe
+ * the identical fallback: `permission-set-drift.ts` (a refused drift-diagnostic
+ * write) and `permission-set-overlay-discard.ts` (a refused resync after a
+ * sanctioned overlay discard). They reuse this spelling and NOT
+ * {@link reportSeedWriteRefusals}, whose PROSE is catalog-seed-specific — see
+ * the deviation note in each of those call sites. Deliberately absent from the
+ * package's `index.ts`: this is an intra-package helper, not public API.
  */
-function logSeedDurabilityFailure(
+export function logSeedDurabilityFailure(
  logger: SeedLogger | undefined,
  message: string,
  meta?: Record<string, any>,

packages/plugins/plugin-security/src/permission-set-drift.test.ts

@@ -24,8 +24,18 @@ import {
 } from './permission-set-drift.js';
 import { permissionSetRowFields } from './permission-set-projection.js';
 
-/** Minimal in-memory ql: sys_permission_set + sys_metadata + $in support. */
-function makeQl(declared: any[] = []) {
+/**
+ * Minimal in-memory ql: sys_permission_set + sys_metadata + $in support.
+ *
+ * `refuseUpdatesWith` makes every `update` throw AFTER the dispatch predicate
+ * has accepted the call shape — the real failure ORDER (the engine validates,
+ * the store refuses), and the only one `tryUpdate`'s catch ever sees. Placing
+ * the throw first would let a call shape the real engine rejects pass this
+ * suite. Extended in place rather than added as a second double, deliberately:
+ * this file's `update` double is pinned 1-per-file in
+ * `scripts/engine-double-contract.pinned.json`.
+ */
+function makeQl(declared: any[] = [], refusal: { refuseUpdatesWith?: Error } = {}) {
  const permRows: any[] = [];
  const metaRows: any[] = [];
  const tableFor = (object: string) =>
@@ -63,6 +73,7 @@ function makeQl(declared: any[] = []) {
  async update(object: string, data: any, options?: any) {
  const rows = tableFor(object);
  const dispatch = assertEngineUpdateDispatch(data, options);
+ if (refusal.refuseUpdatesWith) throw refusal.refuseUpdatesWith;
  const targets = dispatch.kind === 'by-id'
  ? (rows ?? []).filter((r: any) => r.id === dispatch.id)
  : (rows ?? []).filter((r: any) => matches(r, options?.where));
@@ -207,3 +218,109 @@ describe('persistPermissionSetDriftDiagnostics — writes are equality-gated', (
  expect(second.updated).toBe(0); // nothing changed — no round trip
  });
 });
+
+
+/* ------------------------------------------------------------------------- *
+ * pin 6 — a REFUSED diagnostic write must be LOUD
+ *
+ * The defect: `persistPermissionSetDriftDiagnostics` counted only the writes
+ * that LANDED, and `runPermissionSetDriftDiagnostics` reported only when that
+ * count was non-zero. So a boot on which every drift write was refused
+ * computed the drift correctly, persisted none of it, and printed NOTHING —
+ * byte-identical to a deployment with no drift, while the drifted sets kept
+ * enforcing grants that differ from the shipped artifact.
+ *
+ * Driver-error spellings are taken from the shipped classifier's own measured
+ * fixtures (see `seed-write-refusal.test.ts`), never invented here.
+ * ------------------------------------------------------------------------- */
+
+/** Records every channel separately, with `error`'s real 3-arg shape. */
+function recordingLogger() {
+ const info: any[] = [];
+ const warn: any[] = [];
+ const error: any[] = [];
+ return {
+ info, warn, error,
+ sink: {
+ info: (m: string, meta?: any) => { info.push({ m, meta }); },
+ warn: (m: string, meta?: any) => { warn.push({ m, meta }); },
+ error: (m: string, e?: any, meta?: any) => { error.push({ m, e, meta }); },
+ },
+ };
+}
+
+/** NOT a unique violation — the realistic refusal for an update-by-id. */
+const connectionFailure = () =>
+ Object.assign(new Error('connect ECONNREFUSED 127.0.0.1:5432'), { code: 'ECONNREFUSED' });
+
+describe('runPermissionSetDriftDiagnostics — pin 6: a refused diagnostic write is visible', () => {
+ it('a pass whose every write is REFUSED still reports — the drifted set is named, and the refusal is reported on the durability channel', async () => {
+ const artifact = declaredSet();
+ const ql = makeQl([artifact], { refuseUpdatesWith: connectionFailure() });
+ ql.permRows.push(inSyncRow({
+ managed_by: 'admin', // provenance_skip — real drift
+ object_permissions: JSON.stringify({ obj_a: { allowRead: true } }),
+ }));
+ const log = recordingLogger();
+
+ const result = await runPermissionSetDriftDiagnostics(ql, { logger: log.sink });
+
+ // The drift was computed correctly and NONE of it landed.
+ expect(result.diagnostics[0].status).toBe('provenance_skip');
+ expect(result.updated).toBe(0);
+ expect(result.refused).toBe(1);
+ expect(ql.permRows[0].drift_status).toBeUndefined();
+
+ // ⭐ The boot is no longer indistinguishable from a clean one.
+ expect(log.error).toHaveLength(1);
+ expect(log.error[0].m).toContain('REFUSED');
+ expect(log.error[0].meta.refused).toBe(1);
+ expect(log.error[0].meta.refusals[0]).toMatchObject({
+ object: 'sys_permission_set', class: 'other', count: 1,
+ });
+ // The value-free code channel, never the bound statement.
+ expect(log.error[0].meta.refusals[0].driverCodes).toContain('ECONNREFUSED');
+ // Never the catalog-seed prose: this pass seeds nothing, and sending an
+ // operator after a legacy platform-wide catalog index would be a
+ // confident wrong answer. See `reportDriftWriteRefusals`.
+ expect(log.error[0].m).not.toContain('RBAC catalog');
+
+ // …and the line that NAMES the drifted set is no longer gated behind the
+ // counter the refusal suppressed.
+ const drift = log.warn.find((l) => l.m.includes('differ from the shipped artifact'));
+ expect(drift).toBeDefined();
+ expect(drift!.meta.drifted).toEqual([{ name: 'ehr_quality_inspector', status: 'provenance_skip' }]);
+ expect(drift!.meta.updated).toBe(0);
+ expect(drift!.meta.refused).toBe(1);
+ });
+
+ it('⭐ counter-direction: a steady-state pass (nothing to write, nothing refused) still prints NOTHING', async () => {
+ const ql = makeQl([declaredSet()]);
+ ql.permRows.push(inSyncRow({ drift_status: null, drift_detail: null }));
+ const log = recordingLogger();
+
+ const result = await runPermissionSetDriftDiagnostics(ql, { logger: log.sink });
+
+ expect(result.updated).toBe(0);
+ expect(result.refused).toBe(0);
+ // The new `refused > 0` limb re-opens the refusal case and nothing else —
+ // a quiet boot stays exactly as quiet as it was.
+ expect(log.error).toHaveLength(0);
+ expect(log.warn).toHaveLength(0);
+ });
+
+ it('against a REDUCED sink with no `error`, the refusal still prints — at `warn`, never nowhere', async () => {
+ const ql = makeQl([declaredSet()], { refuseUpdatesWith: connectionFailure() });
+ ql.permRows.push(inSyncRow({
+ managed_by: 'admin',
+ object_permissions: JSON.stringify({ obj_a: { allowRead: true } }),
+ }));
+ const warn: any[] = [];
+
+ // `{ warn }` alone is a legal ProjectionLogger — hosts do inject reduced
+ // sinks, which is exactly why the durability fallback is mandatory.
+ await runPermissionSetDriftDiagnostics(ql, { logger: { warn: (m: string, meta?: any) => { warn.push({ m, meta }); } } });
+
+ expect(warn.some((l) => l.m.includes('REFUSED'))).toBe(true);
+ });
+});

packages/plugins/plugin-security/src/permission-set-drift.ts

@@ -62,6 +62,10 @@ import {
  tryUpdate,
  type ProjectionLogger,
 } from './permission-set-projection.js';
+import {
+ createSeedWriteRefusals,
+ logSeedDurabilityFailure,
+} from './per-organization-catalog.js';
 import { readDeclared } from './bootstrap-declared-permissions.js';
 import { buildExistingByName } from './seed-name-lookup.js';
 
@@ -189,6 +193,75 @@ export async function computePermissionSetDriftDiagnostics(
  return out;
 }
 
+/**
+ * Report, ONCE per pass, the drift-diagnostic writes the store refused.
+ *
+ * ## Why this is not {@link reportSeedWriteRefusals}
+ *
+ * The shared ACCUMULATOR (`createSeedWriteRefusals`) is reused as-is — it
+ * carries the shipped `isUniqueViolationError` classification and the
+ * value-free `code`/`errno` channel, and re-deriving either here is exactly
+ * the local-regex defect that module was written to retire. The shared
+ * REPORTER is not, and deliberately: every sentence it prints is about
+ * seeding the RBAC catalog — "the catalog is INCOMPLETE", "this pass's
+ * 'seeded' count", and a remedy naming the legacy PLATFORM-WIDE unique index
+ * on the catalog name column. None of that is true here. This pass seeds
+ * nothing; it writes two diagnostic columns onto rows that already exist, by
+ * id. Printing that text over this failure would send an operator to
+ * `os migrate` for a defect that is not there — the same "a confident wrong
+ * answer is worse than no answer" reasoning that makes `other` its own class
+ * in `reportSeedWriteRefusals` rather than a relabelled unique violation.
+ *
+ * ## Why ONE line and not one per class
+ *
+ * `reportSeedWriteRefusals` splits its classes because their REMEDIES differ.
+ * Here they do not: an update-by-id of two nullable diagnostic columns has no
+ * unique constraint to violate, so the class is a fact for `meta` (it travels
+ * there, per (object, class), with the driver codes) and not a reason to print
+ * a second sentence.
+ *
+ * ## Why the durability channel
+ *
+ * AGENTS.md "Degradation log levels", one question — after the degradation,
+ * does the system still look normal while something it claims is persisted
+ * has not landed? Yes, precisely: `drift_status` / `drift_detail` are what
+ * Setup's "Needs Attention" surface reads, so a refused write leaves that
+ * screen showing a CLEAN environment over sets that are still drifted.
+ * ⚠️ `check:durability-log-level` does not vouch for this choice — `ql.update`
+ * is not in its `DURABILITY_CRITICAL_CALLEES` vocabulary, so its green here
+ * means the site is outside the gate's reach (NOT MEASURED), never approval.
+ */
+function reportDriftWriteRefusals(
+ logger: ProjectionLogger | undefined,
+ refusals: ReturnType<typeof createSeedWriteRefusals>,
+ organizationId?: string,
+): void {
+ const entries = refusals.report();
+ if (entries.length === 0) return;
+ logSeedDurabilityFailure(
+ logger,
+ `[security] ${refusals.total} package-declared permission set drift diagnostic(s) were REFUSED by the ` +
+ `store — the "declared ≠ enforced" verdict this pass computed did NOT persist, so those rows keep ` +
+ `their PREVIOUS drift_status/drift_detail (usually none at all), Setup's "Needs Attention" surface goes ` +
+ `on showing them as CLEAN, and THE DEPLOYMENT WILL GO ON LOOKING HEALTHY while those sets keep ` +
+ `enforcing grants that differ from the shipped artifact. The drifted set NAMES are not lost with the ` +
+ `write: the "[security] package-declared permission set(s) enforcing grants that differ from the ` +
+ `shipped artifact" line is logged alongside this one and names every one of them — it is no longer ` +
+ `gated behind the count of writes that landed. What the store actually said is in the query engine's ` +
+ `"Update operation failed" entries logged just before this one, which keep the driver's own identifier ` +
+ `with the bound statement and its values cut. Remedy: read those entries — a sys_permission_set ` +
+ `missing its drift_status/drift_detail columns is a deployment SCHEMA defect, reported by ` +
+ `"os migrate plan" and fixed by "os migrate apply"; anything else is a store outage, which this pass ` +
+ `re-computes and re-attempts on the next boot. Either way nothing is lost, and nothing is recorded ` +
+ `either until a write lands.`,
+ {
+ refused: refusals.total,
+ refusals: entries,
+ ...(organizationId ? { organization: organizationId } : {}),
+ },
+ );
+}
+
 /**
  * Write the computed diagnostics onto their `sys_permission_set` rows.
  * EQUALITY-GATED (#10946 discipline): a row whose stored `drift_status` /
@@ -200,40 +273,66 @@ export async function computePermissionSetDriftDiagnostics(
  * fact, not merely a filtered view: a quiet set carries no `drift_status`
  * value at all, so a client reading the raw record (not only the Setup
  * "Needs Attention" view) sees nothing to worry about either.
+ *
+ * Answers `refused` alongside `updated`. ⚠️ They are not two spellings of the
+ * same pass: `updated` counts the verdicts that LANDED and `refused` counts
+ * the ones the store rejected, and a caller asking "was there drift?" while
+ * reading only `updated` reads a wholly refused pass as a clean one — the
+ * defect {@link reportDriftWriteRefusals} and the gate in
+ * {@link runPermissionSetDriftDiagnostics} exist to close.
  */
 export async function persistPermissionSetDriftDiagnostics(
  ql: any,
  diagnostics: readonly PermissionSetDriftDiagnostic[],
  opts: DriftDiagnosticsOptions = {},
-): Promise<{ updated: number }> {
+): Promise<{ updated: number; refused: number }> {
+ // ⛔ The refusal LOG is what makes a refused write distinguishable from
+ // "nothing to write". `tryUpdate` answers `false` for both, and this
+ // function's only output used to be a count of the writes that LANDED — so
+ // a pass whose every write was refused returned `{ updated: 0 }`, which is
+ // byte-identical to the steady-state boot the equality gate above is built
+ // to produce. See the report below for what that silence cost.
+ const refusals = createSeedWriteRefusals();
  let updated = 0;
  for (const d of diagnostics) {
  const status: string | null = d.status === 'in_sync' ? null : d.status;
  const detail: string | null = d.status === 'in_sync' ? null : d.detail;
  if (d.priorStatus === status && d.priorDetail === detail) continue;
- if (await tryUpdate(ql, 'sys_permission_set', { id: d.id, drift_status: status, drift_detail: detail }, opts.organizationId)) {
+ if (await tryUpdate(ql, 'sys_permission_set', { id: d.id, drift_status: status, drift_detail: detail }, opts.organizationId, refusals)) {
  updated += 1;
  }
  }
- return { updated };
+ reportDriftWriteRefusals(opts.logger, refusals, opts.organizationId);
+ return { updated, refused: refusals.total };
 }
 
 /** Compute + persist in one call — what boot wiring uses. */
 export async function runPermissionSetDriftDiagnostics(
  ql: any,
  opts: DriftDiagnosticsOptions = {},
-): Promise<{ diagnostics: PermissionSetDriftDiagnostic[]; updated: number }> {
+): Promise<{ diagnostics: PermissionSetDriftDiagnostic[]; updated: number; refused: number }> {
  const diagnostics = await computePermissionSetDriftDiagnostics(ql, opts);
- const { updated } = await persistPermissionSetDriftDiagnostics(ql, diagnostics, opts);
- if (updated > 0) {
+ const { updated, refused } = await persistPermissionSetDriftDiagnostics(ql, diagnostics, opts);
+ // ⛔ `updated > 0` ALONE was the suppressor. A boot on which every drift
+ // write is refused computes the drift correctly, persists none of it, and —
+ // under the old gate — printed nothing at all, which is byte-identical to a
+ // deployment with no drift. `refused > 0` re-opens exactly that case and
+ // nothing else: a steady-state boot (equality-gated, nothing to write,
+ // nothing refused) stays as quiet as it was.
+ if (updated > 0 || refused > 0) {
  opts.logger?.warn?.(
  '[security] package-declared permission set(s) enforcing grants that differ from the shipped artifact',
  {
  updated,
+ // Present ONLY when non-zero, so the steady-state line is unchanged
+ // byte-for-byte for anything reading it. Read it together with
+ // `updated`: `updated` counts the verdicts that LANDED, never the
+ // verdicts that were reached.
+ ...(refused > 0 ? { refused } : {}),
  drifted: diagnostics.filter((d) => d.status !== 'in_sync').map((d) => ({ name: d.name, status: d.status })),
  ...(opts.organizationId ? { organization: opts.organizationId } : {}),
  },
  );
  }
- return { diagnostics, updated };
+ return { diagnostics, updated, refused };
 }