diff --git a/.changeset/optional-error-sink-paydown.md b/.changeset/optional-error-sink-paydown.md new file mode 100644 index 0000000000..c4fcdfbdd7 --- /dev/null +++ b/.changeset/optional-error-sink-paydown.md @@ -0,0 +1,64 @@ +--- +"@objectstack/cloud-connection": minor +"@objectstack/metadata-protocol": minor +"@objectstack/plugin-approvals": minor +"@objectstack/plugin-audit": minor +"@objectstack/plugin-auth": minor +"@objectstack/plugin-email": minor +"@objectstack/plugin-reports": minor +"@objectstack/plugin-sharing": minor +"@objectstack/plugin-webhooks": minor +"@objectstack/service-knowledge": minor +--- + +**BREAKING** (compile-time only): twelve logger sink types that declared an +optional `error` now declare a **non-optional** `warn`, so a durability report +always has somewhere to land (#9754, #10556). + +`minor`, not `major`: during the launch window this stack ships breaking changes +as `minor` — every publishable package versions in lockstep, so a `major` would +promote the whole release. `patch` would be wrong in the other direction, because +this *can* break a consumer's build. + +`error` stays optional on every one of these types — hosts legitimately inject +reduced sinks, and requiring `error` was measured and rejected as #9754 option C. +What changes is that its *absence* now has a declared, guaranteed destination. +Call sites keep the `logger?.warn?.(…)` spelling as the backstop for hosts the +type cannot reach, so **no runtime behaviour changes**: nothing that printed +before stops printing, and nothing silent starts printing. + +### Who has to change, and what to do + +Only a caller that hands one of these sinks an object with **no `warn` method** — +for example `{ info }` or `{ error }` alone. Add a `warn` member; there is no +rename, no removal, and no stored value or metadata key to rewrite. Every +construction site inside this repo already supplied one, so the in-repo cost was +zero; the compile error is reserved for the callers that were silently discarding +these reports. + +The affected types, by package: + +- `@objectstack/cloud-connection` — the internal `PluginContext['logger']` +- `@objectstack/metadata-protocol` — `IndexMigrationLogger` +- `@objectstack/plugin-approvals` — the internal `MinimalLogger` of `lifecycle-hooks` +- `@objectstack/plugin-audit` — `AuthEventAuditLogger`, `ReadAuditLogger` +- `@objectstack/plugin-auth` — `ReconcileMembershipDeps['logger']`, the internal + `LoggerLike` of `member-role-canonical`, and `AuthManagerOptions['logger']` +- `@objectstack/plugin-email` — `ReclaimLogger`, via `ReclaimAttachmentContentOptions` +- `@objectstack/plugin-reports` — `ReportServiceOptions['logger']` +- `@objectstack/plugin-sharing` — the internal `MinimalLogger` of `bulk-recompute`, + `rule-hooks` and `record-share-cascade` +- `@objectstack/plugin-webhooks` — `OptionalLogger`, via `AutoEnqueuerOptions` +- `@objectstack/service-knowledge` — `KnowledgeLogger` + +`AuthManagerOptions['logger']` is the one most likely to be reached from outside: +`AuthManager` is public surface, its `logger` option stays optional, and a logger +that *is* supplied must now carry `warn`. The only non-test construction site in +this repo passes the kernel `Logger`, whose `warn` is already required. + +`ReportService` and `AutoEnqueuer` additionally stopped defaulting their logger +field to `{}`. The field is now honestly optional rather than holding an empty +object that declared it could report and discarded everything. Behaviour is +unchanged in both directions. + + diff --git a/packages/cloud-connection/src/cloud-connection-plugin.ts b/packages/cloud-connection/src/cloud-connection-plugin.ts index 6b703c171f..46ad61b098 100644 --- a/packages/cloud-connection/src/cloud-connection-plugin.ts +++ b/packages/cloud-connection/src/cloud-connection-plugin.ts @@ -56,7 +56,15 @@ interface PluginContext { getService(name: string): T; logger?: { info?: (msg: string) => void; - warn?: (msg: string) => void; + /** + * The GUARANTEED fallback channel (#9754). `error` stays optional — hosts do + * inject reduced sinks — so `warn` is where a durability report lands when + * `error` is absent, and a fallback that may itself be missing is not a + * fallback. Call sites keep the `logger?.warn?.(…)` spelling as the backstop + * for hosts the TYPE cannot reach; `SweepLogger` in plugin-email's + * `outbox-sweep.ts` carries the full reasoning and the measurement. + */ + warn: (msg: string) => void; error?: (msg: string, err?: unknown) => void; }; } diff --git a/packages/metadata-protocol/src/migrations/partial-index-probe.test.ts b/packages/metadata-protocol/src/migrations/partial-index-probe.test.ts index 3bcee65212..771b053a44 100644 --- a/packages/metadata-protocol/src/migrations/partial-index-probe.test.ts +++ b/packages/metadata-protocol/src/migrations/partial-index-probe.test.ts @@ -11,6 +11,7 @@ import { probeThenReplaceIndex, type IndexExec, } from './partial-index-probe.js'; +import type { IndexMigrationLogger } from './partial-index-probe.js'; /** * The probe-first order, tested where it lives (#6418). @@ -382,6 +383,18 @@ describe('probe-first partial index replacement (#6418)', () => { expect(warnOnly.warn).toHaveBeenCalledWith('msg', { detail: 'detail' }); expect(() => logProblem(undefined, 'msg', 'detail')).not.toThrow(); - expect(() => logProblem({}, 'msg', 'detail')).not.toThrow(); + // `{}` is no longer a legal `IndexMigrationLogger` — #9754 made `warn` + // non-optional precisely so a sink with NEITHER channel cannot be + // written. The cast is deliberate and is the point of the case: it + // forces through the one host the TYPE cannot reach (a plain-JS + // embedder, or a cast at the boundary) and pins that `logProblem`'s + // `?.` backstop still degrades to silence instead of throwing + // `logger.warn is not a function` inside a migration probe. Type-level + // guarantee and runtime backstop are different promises; this asserts + // the second one, and the compile error that used to be impossible here + // is now what proves the first. + expect(() => + logProblem({} as unknown as IndexMigrationLogger, 'msg', 'detail'), + ).not.toThrow(); }); }); diff --git a/packages/metadata-protocol/src/migrations/partial-index-probe.ts b/packages/metadata-protocol/src/migrations/partial-index-probe.ts index c5a1f41191..f4fbb2018c 100644 --- a/packages/metadata-protocol/src/migrations/partial-index-probe.ts +++ b/packages/metadata-protocol/src/migrations/partial-index-probe.ts @@ -97,13 +97,24 @@ export function resolveIndexExecForTable(engine: unknown, table: string): IndexE /** * Minimal logger surface, structurally compatible with `@objectstack/spec`'s - * `Logger` (every method optional so a bare console or a test double fits). + * `Logger` (a bare console or a test double fits — but see `warn` below, which + * #9754 made non-optional, so a double must now declare it). * Signatures mirror that contract exactly — notably `error(msg, Error, meta)` * versus `warn(msg, meta)` — so a host `Logger` is assignable as-is. */ export interface IndexMigrationLogger { info?(message: string, meta?: Record): void; - warn?(message: string, meta?: Record): void; + /** + * The GUARANTEED fallback channel (#9754). `error` stays optional — hosts do + * inject reduced sinks — so `warn` is where a durability report lands when + * `error` is absent, and a fallback that may itself be missing is not a + * fallback. `logProblem` below is exactly that degrade, and until this member + * was required it could reach for two channels and find neither. Call sites + * keep the `logger?.warn?.(…)` spelling as the backstop for hosts the TYPE + * cannot reach; `SweepLogger` in plugin-email's `outbox-sweep.ts` carries the + * full reasoning and the measurement. + */ + warn(message: string, meta?: Record): void; error?(message: string, error?: Error, meta?: Record): void; } diff --git a/packages/plugins/plugin-approvals/src/lifecycle-hooks.ts b/packages/plugins/plugin-approvals/src/lifecycle-hooks.ts index 84eb7f93d0..193badf4d2 100644 --- a/packages/plugins/plugin-approvals/src/lifecycle-hooks.ts +++ b/packages/plugins/plugin-approvals/src/lifecycle-hooks.ts @@ -87,7 +87,15 @@ interface MinimalEngine { interface MinimalLogger { debug?: (msg: any, ...rest: any[]) => void; info?: (msg: any, ...rest: any[]) => void; - warn?: (msg: any, ...rest: any[]) => void; + /** + * The GUARANTEED fallback channel (#9754). `error` stays optional — hosts do + * inject reduced sinks — so `warn` is where a durability report lands when + * `error` is absent, and a fallback that may itself be missing is not a + * fallback. Call sites keep the `logger?.warn?.(…)` spelling as the backstop + * for hosts the TYPE cannot reach; `SweepLogger` in plugin-email's + * `outbox-sweep.ts` carries the full reasoning and the measurement. + */ + warn: (msg: any, ...rest: any[]) => void; error?: (msg: any, ...rest: any[]) => void; } diff --git a/packages/plugins/plugin-audit/src/auth-event-audit.test.ts b/packages/plugins/plugin-audit/src/auth-event-audit.test.ts index e0ed6e612a..aed06adc65 100644 --- a/packages/plugins/plugin-audit/src/auth-event-audit.test.ts +++ b/packages/plugins/plugin-audit/src/auth-event-audit.test.ts @@ -304,13 +304,20 @@ describe('[#8144] createAuthEventAuditSink writes a login row that names its act throw new Error('no such table: sys_audit_log'); }, }; - const logger = { error: vi.fn(), debug: vi.fn() }; + // `warn` is required by `AuthEventAuditLogger` (#9754/#10556): a sink that + // declares an optional `error` must be able to degrade. Present here AND + // asserted unused below, which pins the ORDER — `error` first, `warn` only + // as the fallback. Before the contract change this double was `{ error, + // debug }`, a shape the type accepted and the degrade path could not use. + const logger = { error: vi.fn(), warn: vi.fn(), debug: vi.fn() }; const sink = createAuthEventAuditSink({ getEngine: () => broken, logger }); await expect(sink.recordAuthEvent({ action: 'login', userId: 'usr_1' })).resolves.toBeUndefined(); await sink.recordAuthEvent({ action: 'logout', userId: 'usr_1' }); expect(logger.error).toHaveBeenCalledTimes(1); + // The fallback stays untouched while `error` exists. + expect(logger.warn).not.toHaveBeenCalled(); const [msg] = logger.error.mock.calls[0]; // The two things a durability `error` owes, in its first line. expect(String(msg)).toContain('INCOMPLETE'); diff --git a/packages/plugins/plugin-audit/src/auth-event-audit.ts b/packages/plugins/plugin-audit/src/auth-event-audit.ts index e88ef35cdb..01f65820ed 100644 --- a/packages/plugins/plugin-audit/src/auth-event-audit.ts +++ b/packages/plugins/plugin-audit/src/auth-event-audit.ts @@ -106,13 +106,19 @@ export interface AuthSessionAuditEvent { export interface AuthEventAuditLogger { error?(msg: string, err?: Error, meta?: Record): void; /** - * The fallback channel for the durability report below. `error` is optional - * here, so a sink that has none must still have somewhere to put a lost audit - * row — reaching for `error` and finding nothing must degrade to `warn`, - * never to silence (#9657). Signature and optionality mirror - * `ReadAuditLogger` in `read-audit.ts`, which already declared it. + * The GUARANTEED fallback channel for the durability report below. `error` is + * optional here, so a sink that has none must still have somewhere to put a + * lost audit row — reaching for `error` and finding nothing must degrade to + * `warn`, never to silence (#9657). + * + * This is the sink #9754's body calls the sharpest instance: it declared + * `error?` and `debug?` and NO `warn` at all, so the call site below COULD NOT + * have been written correctly against the contract it was given. #9750 added + * `warn?`, which gave it something to reach for and still no guarantee it was + * there; non-optional (#9754) is what makes the silence unrepresentable. + * Signature and optionality mirror `ReadAuditLogger` in `read-audit.ts`. */ - warn?(msg: string, meta?: Record): void; + warn(msg: string, meta?: Record): void; debug?(msg: string, meta?: Record): void; } diff --git a/packages/plugins/plugin-audit/src/read-audit.test.ts b/packages/plugins/plugin-audit/src/read-audit.test.ts index 32add0e33b..c2f718a9f4 100644 --- a/packages/plugins/plugin-audit/src/read-audit.test.ts +++ b/packages/plugins/plugin-audit/src/read-audit.test.ts @@ -314,10 +314,18 @@ describe('#8992 the write is OFF the request path', () => { it('a ledger write failure never reaches the read, and reports once at `error`', async () => { const errors: string[] = []; const debugs: string[] = []; + // Required by `ReadAuditLogger` (#9754/#10556) — and asserted unused below, + // so the double pins that `error` is reached for FIRST and `warn` is only + // the degrade path. + const warns: string[] = []; const writer = installReadAuditWriter(engine, { objects: ['contact'], timers: makeManualTimers(), - logger: { error: (m: string) => errors.push(m), debug: (m: string) => debugs.push(m) }, + logger: { + error: (m: string) => errors.push(m), + warn: (m: string) => warns.push(m), + debug: (m: string) => debugs.push(m), + }, })!; // Break the ledger AFTER install, so the probe has already run. (engine as any).insert = async () => { throw new Error('no such table: sys_audit_log'); }; diff --git a/packages/plugins/plugin-audit/src/read-audit.ts b/packages/plugins/plugin-audit/src/read-audit.ts index c3e12d333e..1eb727ee2a 100644 --- a/packages/plugins/plugin-audit/src/read-audit.ts +++ b/packages/plugins/plugin-audit/src/read-audit.ts @@ -112,7 +112,15 @@ export const READ_AUDIT_ACTION = 'read'; /** Minimal logger surface — structurally the kernel `ctx.logger` (`ILogger`). */ export interface ReadAuditLogger { error?(msg: string, err?: Error, meta?: Record): void; - warn?(msg: string, meta?: Record): void; + /** + * The GUARANTEED fallback channel (#9754). `error` stays optional — hosts do + * inject reduced sinks — so `warn` is where a durability report lands when + * `error` is absent, and a fallback that may itself be missing is not a + * fallback. Call sites keep the `logger?.warn?.(…)` spelling as the backstop + * for hosts the TYPE cannot reach; `SweepLogger` in plugin-email's + * `outbox-sweep.ts` carries the full reasoning and the measurement. + */ + warn(msg: string, meta?: Record): void; debug?(msg: string, meta?: Record): void; } diff --git a/packages/plugins/plugin-auth/src/auth-manager.ts b/packages/plugins/plugin-auth/src/auth-manager.ts index 24e74d27e8..2b52267049 100644 --- a/packages/plugins/plugin-auth/src/auth-manager.ts +++ b/packages/plugins/plugin-auth/src/auth-manager.ts @@ -631,8 +631,20 @@ export interface AuthManagerOptions extends Partial { * Optional structured logger (the kernel `ctx.logger`) for best-effort * bookkeeping surfaces such as the ADR-0093 membership reconciler. Omitted → * those surfaces run silently (they already fail closed to no-op). + * + * The whole field stays OPTIONAL — omitting it is still a supported posture. + * What is no longer representable is supplying a logger that cannot carry a + * durability report: `warn` is non-optional because this value is FORWARDED + * verbatim into `ReconcileMembershipDeps.logger` (reconcile-membership.ts), + * whose sink guarantees that channel under #9754. Measured before tightening: + * the only non-test construction site in this repo is `auth-plugin.ts`, which + * passes `ctx.logger` — the kernel `Logger`, whose `warn` is already + * required — so the in-tree cost is zero. An external embedder handing + * `AuthManager` a reduced `{ info }` sink is the one caller this asks to + * change, and that is the point: it was the caller silently discarding the + * reconciler's reports (#10556). */ - logger?: { info?: (msg: string, meta?: any) => void; warn?: (msg: string, meta?: any) => void }; + logger?: { info?: (msg: string, meta?: any) => void; warn: (msg: string, meta?: any) => void }; /** * ADR-0069 D2 — account lockout (anti-brute-force). After this many diff --git a/packages/plugins/plugin-auth/src/member-role-canonical.ts b/packages/plugins/plugin-auth/src/member-role-canonical.ts index 2015eadee5..c65741a033 100644 --- a/packages/plugins/plugin-auth/src/member-role-canonical.ts +++ b/packages/plugins/plugin-auth/src/member-role-canonical.ts @@ -198,7 +198,15 @@ export function isCanonicalMemberRole(raw: unknown): boolean { */ type LoggerLike = { info?(msg: string, meta?: Record): void; - warn?(msg: string, meta?: Record): void; + /** + * The GUARANTEED fallback channel (#9754). `error` stays optional — hosts do + * inject reduced sinks — so `warn` is where a durability report lands when + * `error` is absent, and a fallback that may itself be missing is not a + * fallback. Call sites keep the `logger?.warn?.(…)` spelling as the backstop + * for hosts the TYPE cannot reach; `SweepLogger` in plugin-email's + * `outbox-sweep.ts` carries the full reasoning and the measurement. + */ + warn(msg: string, meta?: Record): void; error?(msg: string, error?: Error, meta?: Record): void; debug?(msg: string, meta?: Record): void; }; diff --git a/packages/plugins/plugin-auth/src/reconcile-membership.test.ts b/packages/plugins/plugin-auth/src/reconcile-membership.test.ts index fb5212b2d2..696fce5bed 100644 --- a/packages/plugins/plugin-auth/src/reconcile-membership.test.ts +++ b/packages/plugins/plugin-auth/src/reconcile-membership.test.ts @@ -251,7 +251,7 @@ describe('reconcileMembership / backfillMemberships — off-vocabulary policy (# const res = await reconcileMembership(makeEngine(), 'user-1', { policy: asPolicy('inviteOnly'), resolveTargetOrg: async () => 'org_default', - logger: { error }, + logger: { error, warn: vi.fn() }, }); // Returned, so the diagnosis survives a caller that passed no logger. expect(res.error).toContain(`'inviteOnly'`); @@ -302,7 +302,7 @@ describe('reconcileMembership / backfillMemberships — off-vocabulary policy (# // fields that have no business in a log line. policy: asPolicy({ membershipPolicy: 'invite-only', adminEmail: 'ops@example.com' }), resolveTargetOrg: async () => 'org_default', - logger: { error }, + logger: { error, warn: vi.fn() }, }); expect(res.outcome).toBe('invalid-policy'); expect(res.error).toContain('[object]'); @@ -315,7 +315,7 @@ describe('reconcileMembership / backfillMemberships — off-vocabulary policy (# const res = await reconcileMembership(makeEngine(), 'user-1', { policy: asPolicy('x'.repeat(500)), resolveTargetOrg: async () => 'org_default', - logger: { error }, + logger: { error, warn: vi.fn() }, }); expect(res.error).toContain('(truncated)'); expect(res.error!.length).toBeLessThan(200); diff --git a/packages/plugins/plugin-auth/src/reconcile-membership.ts b/packages/plugins/plugin-auth/src/reconcile-membership.ts index af10cc5a84..6806cbf65b 100644 --- a/packages/plugins/plugin-auth/src/reconcile-membership.ts +++ b/packages/plugins/plugin-auth/src/reconcile-membership.ts @@ -106,7 +106,15 @@ export interface ReconcileMembershipDeps { resolveTargetOrg: () => Promise; logger?: { info?: (msg: string, meta?: any) => void; - warn?: (msg: string, meta?: any) => void; + /** + * The GUARANTEED fallback channel (#9754). `error` stays optional — hosts do + * inject reduced sinks — so `warn` is where a durability report lands when + * `error` is absent, and a fallback that may itself be missing is not a + * fallback. Call sites keep the `logger?.warn?.(…)` spelling as the backstop + * for hosts the TYPE cannot reach; `SweepLogger` in plugin-email's + * `outbox-sweep.ts` carries the full reasoning and the measurement. + */ + warn: (msg: string, meta?: any) => void; error?: (msg: string, meta?: any) => void; }; } diff --git a/packages/plugins/plugin-email/src/attachment-reclaim.ts b/packages/plugins/plugin-email/src/attachment-reclaim.ts index c8d6820ee7..79e24d9f34 100644 --- a/packages/plugins/plugin-email/src/attachment-reclaim.ts +++ b/packages/plugins/plugin-email/src/attachment-reclaim.ts @@ -111,7 +111,15 @@ export interface AttachmentReclaimEngine { /** Structural logger — same shape the outbox sweep uses. */ interface ReclaimLogger { info?: (msg: string, meta?: any) => void; - warn?: (msg: string, meta?: any) => void; + /** + * The GUARANTEED fallback channel (#9754). `error` stays optional — hosts do + * inject reduced sinks — so `warn` is where a durability report lands when + * `error` is absent, and a fallback that may itself be missing is not a + * fallback. Call sites keep the `logger?.warn?.(…)` spelling as the backstop + * for hosts the TYPE cannot reach; `SweepLogger` in plugin-email's + * `outbox-sweep.ts` carries the full reasoning and the measurement. + */ + warn: (msg: string, meta?: any) => void; error?: (msg: string, meta?: any) => void; } diff --git a/packages/plugins/plugin-reports/src/report-service.ts b/packages/plugins/plugin-reports/src/report-service.ts index 34a28426a5..469076fead 100644 --- a/packages/plugins/plugin-reports/src/report-service.ts +++ b/packages/plugins/plugin-reports/src/report-service.ts @@ -218,7 +218,14 @@ export interface ReportServiceOptions { engine: ReportEngine; email?: ReportEmail; clock?: ReportClock; - logger?: { info?: (msg: any, ...rest: any[]) => void; warn?: (msg: any, ...rest: any[]) => void; error?: (msg: any, ...rest: any[]) => void }; + /** + * `warn` is GUARANTEED, `error` is not (#9754). Hosts do inject reduced sinks, + * so `error` stays optional — but a durability report that degrades to `warn` + * needs `warn` to actually be there, and this service degrades to it in eight + * places below. The whole `logger` stays optional; what is no longer + * representable is a sink that HAS the field and still prints nothing. + */ + logger?: { info?: (msg: any, ...rest: any[]) => void; warn: (msg: any, ...rest: any[]) => void; error?: (msg: any, ...rest: any[]) => void }; /** Cap rows per report to protect both DB and email size. */ maxRows?: number; /** @@ -260,7 +267,22 @@ export class ReportService implements IReportService { private readonly engine: ReportEngine; private readonly email?: ReportEmail; private readonly clock: ReportClock; - private readonly logger: NonNullable; + /** + * Optional, and deliberately NOT defaulted to `{}` (#10556). + * + * `ReportServiceOptions['logger']` guarantees a `warn` channel under #9754, so `{}` stopped being a + * legal value of the type — which is the gate working: an empty object is a + * sink that declares it can report and then discards everything. The repair + * is to say what is TRUE — there may be no logger at all — rather than to + * mint a sink that lies. Runtime behaviour is unchanged in both directions: + * absent logger and `{}` both printed nothing before, and print nothing now. + * + * ⛔ What this deliberately does NOT decide: whether an absent host sink should + * instead default to a `console`-backed one. That is the open design call the + * #9754 ledger records against `plugin-security`'s `= {}` field, and it is a + * maintainer decision — not something to settle here to make a checker green. + */ + private readonly logger?: ReportServiceOptions['logger']; private readonly maxRows: number; private readonly resolveOwnerContext?: OwnerContextResolver; private readonly canExportFn?: (object: string, context: unknown) => Promise; @@ -269,7 +291,7 @@ export class ReportService implements IReportService { this.engine = opts.engine; this.email = opts.email; this.clock = opts.clock ?? { now: () => new Date() }; - this.logger = opts.logger ?? {}; + this.logger = opts.logger; this.maxRows = Math.max(1, opts.maxRows ?? 5000); this.resolveOwnerContext = opts.resolveOwnerContext; this.canExportFn = opts.canExport; @@ -306,7 +328,7 @@ export class ReportService implements IReportService { try { allowed = await this.canExportFn(object, context); } catch (err) { - this.logger.warn?.('ReportService: canExport check failed — denying export', err); + this.logger?.warn?.('ReportService: canExport check failed — denying export', err); allowed = false; } if (!allowed) { @@ -529,7 +551,7 @@ export class ReportService implements IReportService { updated_at: ranAt, }, { context: SYSTEM_CTX }); } catch (err) { - this.logger.warn?.('ReportService: failed to stamp last_run_at', err); + this.logger?.warn?.('ReportService: failed to stamp last_run_at', err); } } @@ -688,7 +710,7 @@ export class ReportService implements IReportService { const ownerId = report.owner_id; const runContext = ownerId && this.resolveOwnerContext ? await this.resolveOwnerContext(ownerId).catch((err) => { - this.logger.warn?.('ReportService.dispatchDue: owner context resolution failed', err); + this.logger?.warn?.('ReportService.dispatchDue: owner context resolution failed', err); return null; }) : null; @@ -742,7 +764,7 @@ export class ReportService implements IReportService { }); } } else if (!this.email) { - this.logger.warn?.('ReportService.dispatchDue: no email service — schedule fired but mail not sent'); + this.logger?.warn?.('ReportService.dispatchDue: no email service — schedule fired but mail not sent'); } await this.advanceSchedule(schedule, ts); @@ -753,7 +775,7 @@ export class ReportService implements IReportService { last_status: 'failed', last_error: String(err?.message ?? err ?? 'unknown').slice(0, 500), }); - this.logger.error?.('ReportService.dispatchDue: schedule failed', err); + this.logger?.error?.('ReportService.dispatchDue: schedule failed', err); } } return { fired, failed, skipped }; @@ -778,9 +800,9 @@ export class ReportService implements IReportService { try { const next = new Cron(cron, { timezone: schedule.timezone || 'UTC' }).nextRun(from); if (next) return next; - this.logger.warn?.(`ReportService: cron '${cron}' has no next occurrence; falling back to interval`); + this.logger?.warn?.(`ReportService: cron '${cron}' has no next occurrence; falling back to interval`); } catch (err) { - this.logger.warn?.(`ReportService: invalid cron '${cron}'; falling back to interval`, err); + this.logger?.warn?.(`ReportService: invalid cron '${cron}'; falling back to interval`, err); } } const interval = schedule.interval_minutes ?? DEFAULT_INTERVAL_MIN; @@ -805,7 +827,7 @@ export class ReportService implements IReportService { id, ...patch, updated_at: this.clock.now().toISOString(), }, { context: SYSTEM_CTX }); } catch (err) { - this.logger.warn?.('ReportService: failed to mark schedule', err); + this.logger?.warn?.('ReportService: failed to mark schedule', err); } } } diff --git a/packages/plugins/plugin-sharing/src/bulk-recompute.ts b/packages/plugins/plugin-sharing/src/bulk-recompute.ts index f1adf3a6fb..65d6dcf6ea 100644 --- a/packages/plugins/plugin-sharing/src/bulk-recompute.ts +++ b/packages/plugins/plugin-sharing/src/bulk-recompute.ts @@ -102,7 +102,15 @@ export interface RecomputeEngine { interface MinimalLogger { info?: (msg: any, ...rest: any[]) => void; - warn?: (msg: any, ...rest: any[]) => void; + /** + * The GUARANTEED fallback channel (#9754). `error` stays optional — hosts do + * inject reduced sinks — so `warn` is where a durability report lands when + * `error` is absent, and a fallback that may itself be missing is not a + * fallback. Call sites keep the `logger?.warn?.(…)` spelling as the backstop + * for hosts the TYPE cannot reach; `SweepLogger` in plugin-email's + * `outbox-sweep.ts` carries the full reasoning and the measurement. + */ + warn: (msg: any, ...rest: any[]) => void; error?: (msg: any, ...rest: any[]) => void; } diff --git a/packages/plugins/plugin-sharing/src/record-share-cascade.ts b/packages/plugins/plugin-sharing/src/record-share-cascade.ts index 2d2227fbc4..0a828cfd46 100644 --- a/packages/plugins/plugin-sharing/src/record-share-cascade.ts +++ b/packages/plugins/plugin-sharing/src/record-share-cascade.ts @@ -203,7 +203,14 @@ export interface CascadeEngine { interface MinimalLogger { info?: (msg: any, ...rest: any[]) => void; - warn?: (msg: any, ...rest: any[]) => void; + /** + * Non-optional for the same reason as `rule-hooks.ts`'s twin: this logger is + * FORWARDED into `stashAffectedRows` (bulk-recompute.ts), whose sink + * guarantees a `warn` channel under #9754. A `warn?` here would re-open the + * silence one module downstream of where it was closed, and `tsc` said so the + * moment the callee tightened (#10556). + */ + warn: (msg: any, ...rest: any[]) => void; } /** diff --git a/packages/plugins/plugin-sharing/src/rule-hooks.ts b/packages/plugins/plugin-sharing/src/rule-hooks.ts index 03ff94f37d..027726718c 100644 --- a/packages/plugins/plugin-sharing/src/rule-hooks.ts +++ b/packages/plugins/plugin-sharing/src/rule-hooks.ts @@ -75,7 +75,18 @@ interface MinimalEngine { interface MinimalLogger { info?: (msg: any, ...rest: any[]) => void; - warn?: (msg: any, ...rest: any[]) => void; + /** + * Non-optional because this logger is FORWARDED into `stashAffectedRowsOnCtx` + * (bulk-recompute.ts), whose sink guarantees a `warn` channel under #9754. A + * `warn?` here would re-open the silence one module downstream of the place it + * was closed — the forwarding seam is exactly where a guarantee gets lost, and + * `tsc` reported it the moment the callee's contract tightened (#10556). + * + * This shape declares no `error` at all, so it is not itself in the + * optional-error-sink population; what it must not do is hand a + * silence-permitting value to something that promises otherwise. + */ + warn: (msg: any, ...rest: any[]) => void; } /** diff --git a/packages/plugins/plugin-webhooks/src/auto-enqueuer.ts b/packages/plugins/plugin-webhooks/src/auto-enqueuer.ts index a072941287..9a909cf7dd 100644 --- a/packages/plugins/plugin-webhooks/src/auto-enqueuer.ts +++ b/packages/plugins/plugin-webhooks/src/auto-enqueuer.ts @@ -87,7 +87,15 @@ const CUSTOM_HEADERS_CREDENTIAL: DropReason = { */ interface OptionalLogger { info?(msg: string, meta?: unknown): void; - warn?(msg: string, meta?: unknown): void; + /** + * The GUARANTEED fallback channel (#9754). `error` stays optional — hosts do + * inject reduced sinks — so `warn` is where a durability report lands when + * `error` is absent, and a fallback that may itself be missing is not a + * fallback. Call sites keep the `logger?.warn?.(…)` spelling as the backstop + * for hosts the TYPE cannot reach; `SweepLogger` in plugin-email's + * `outbox-sweep.ts` carries the full reasoning and the measurement. + */ + warn(msg: string, meta?: unknown): void; debug?(msg: string, meta?: unknown): void; error?(msg: string, err?: unknown, meta?: unknown): void; } @@ -186,7 +194,22 @@ export class AutoEnqueuer { private readonly subscriptions = new Map(); private readonly subscriptionsObject: string; private readonly refreshIntervalMs: number; - private readonly logger: OptionalLogger; + /** + * Optional, and deliberately NOT defaulted to `{}` (#10556). + * + * `OptionalLogger` guarantees a `warn` channel under #9754, so `{}` stopped being a + * legal value of the type — which is the gate working: an empty object is a + * sink that declares it can report and then discards everything. The repair + * is to say what is TRUE — there may be no logger at all — rather than to + * mint a sink that lies. Runtime behaviour is unchanged in both directions: + * absent logger and `{}` both printed nothing before, and print nothing now. + * + * ⛔ What this deliberately does NOT decide: whether an absent host sink should + * instead default to a `console`-backed one. That is the open design call the + * #9754 ledger records against `plugin-security`'s `= {}` field, and it is a + * maintainer decision — not something to settle here to make a checker green. + */ + private readonly logger?: OptionalLogger; private subId: string | undefined; private subIdSelfHeal: string | undefined; private refreshTimer: ReturnType | undefined; @@ -215,7 +238,7 @@ export class AutoEnqueuer { ) { this.subscriptionsObject = opts.subscriptionsObject ?? 'sys_webhook'; this.refreshIntervalMs = opts.refreshIntervalMs ?? 60_000; - this.logger = opts.logger ?? {}; + this.logger = opts.logger; } /** @@ -254,7 +277,7 @@ export class AutoEnqueuer { if (this.refreshIntervalMs > 0) { this.refreshTimer = setInterval(() => { this.refresh().catch((err) => - this.logger.warn?.('[webhook-auto-enqueuer] periodic refresh failed', err), + this.logger?.warn?.('[webhook-auto-enqueuer] periodic refresh failed', err), ); }, this.refreshIntervalMs); // Don't keep the process alive solely for this timer. @@ -295,7 +318,7 @@ export class AutoEnqueuer { .catch(() => undefined) .then(() => (this.running ? this.refresh() : undefined)) .catch((err) => - this.logger.warn?.( + this.logger?.warn?.( '[webhook-auto-enqueuer] re-arm after CryptoProvider registration failed', err, ), @@ -321,7 +344,7 @@ export class AutoEnqueuer { where: { active: true }, }); } catch (err) { - this.logger.warn?.( + this.logger?.warn?.( `[webhook-auto-enqueuer] failed to load ${this.subscriptionsObject}`, err, ); @@ -364,7 +387,7 @@ export class AutoEnqueuer { } } - this.logger.debug?.('[webhook-auto-enqueuer] cache refreshed', { + this.logger?.debug?.('[webhook-auto-enqueuer] cache refreshed', { objects: this.subscriptions.size, rows: rows.length, }); @@ -444,7 +467,7 @@ export class AutoEnqueuer { ): void { const meta = { webhook: sub.name, eventId, err: (err as Error)?.message ?? err }; if (!sub.parkedReason) { - this.logger.warn?.(`[webhook-auto-enqueuer] ${verb} failed`, meta); + this.logger?.warn?.(`[webhook-auto-enqueuer] ${verb} failed`, meta); return; } const message = @@ -455,10 +478,10 @@ export class AutoEnqueuer { + `IHttpOutbox.enqueue instead of MessagingService.enqueueHttp — only the messaging seam ` + `routes a parked event to recordUndeliverable(), and the delivery door refuses it ` + `rather than minting a pending row that would be sent UNSIGNED.`; - if (typeof this.logger.error === 'function') { - this.logger.error(message, err, meta); + if (typeof this.logger?.error === 'function') { + this.logger?.error(message, err, meta); } else { - this.logger.warn?.(message, meta); + this.logger?.warn?.(message, meta); } } @@ -523,7 +546,7 @@ export class AutoEnqueuer { const legacy = readLegacySecret(row?.definition_json); if (legacy) { - this.logger.warn?.( + this.logger?.warn?.( `[webhook-auto-enqueuer] webhook '${sub.name}' still carries its signing secret as ` + `CLEARTEXT in definition_json, readable over the data API (#7799). Signing continues ` + `from it; run the boot sweep (migrateLegacyWebhookSecrets) with a CryptoProvider wired ` + @@ -578,7 +601,7 @@ export class AutoEnqueuer { const legacy = readLegacyHeaders(row?.definition_json); if (legacy) { - this.logger.warn?.( + this.logger?.warn?.( `[webhook-auto-enqueuer] webhook '${sub.name}' still carries its custom headers as ` + `CLEARTEXT in definition_json, readable over the data API (#7986) — that map is the ` + `ordinary place an Authorization header goes. Delivery continues from it; run the boot ` + @@ -630,7 +653,7 @@ export class AutoEnqueuer { err: (err as Error)?.message ?? err, }; if (this.droppedForSecret.has(sub.id)) { - this.logger.debug?.( + this.logger?.debug?.( `[webhook-auto-enqueuer] webhook '${sub.name}' is still dropped for an unresolvable ` + `${credential.noun} (${credential.issues})`, meta, @@ -659,10 +682,10 @@ export class AutoEnqueuer { // The logger surface is a subset of console/kernel logger — `error` is // optional on it, so fall back rather than silently losing the report // on a logger that only implements `warn`. - if (typeof this.logger.error === 'function') { - this.logger.error(message, err, meta); + if (typeof this.logger?.error === 'function') { + this.logger?.error(message, err, meta); } else { - this.logger.warn?.(message, meta); + this.logger?.warn?.(message, meta); } } @@ -697,7 +720,7 @@ export class AutoEnqueuer { // silently no-op again. const unknown = normalized.filter((t) => !DISPATCHABLE_WEBHOOK_TRIGGERS.has(t)); if (unknown.length > 0) { - this.logger.warn?.( + this.logger?.warn?.( `[webhook-auto-enqueuer] webhook '${(row.name as string) ?? row.id}' declares trigger(s) the engine never emits: ` + `${unknown.join(', ')} — ignored. Dispatchable triggers: ` + `${[...DISPATCHABLE_WEBHOOK_TRIGGERS].join(', ')}.`, @@ -720,7 +743,7 @@ export class AutoEnqueuer { // (`webhook/without-triggers`) so the boot log greps into the // same docs. Only active rows reach parseRow, so a deliberately // disabled webhook stays warning-free. - this.logger.warn?.( + this.logger?.warn?.( `[webhook-auto-enqueuer] webhook '${(row.name as string) ?? row.id}' has no dispatchable ` + `triggers — it will NEVER fire (rule webhook/without-triggers): there is no manual fire ` + `path (#3196), so this row is dead while looking armed in Setup. Declare ` + @@ -806,7 +829,7 @@ export class AutoEnqueuer { const payload = event.payload ?? {}; const recordId = (payload as { recordId?: unknown }).recordId; if (typeof recordId !== 'string' || recordId === '') { - this.logger.warn?.( + this.logger?.warn?.( '[webhook-auto-enqueuer] dropping off-contract data event: payload is not a DataEvent ' + '(no top-level string `recordId`) — fix the producer', { type: event.type, object: event.object }, @@ -896,7 +919,7 @@ export class AutoEnqueuer { const payload = event.payload ?? {}; const matched = (payload as { matched?: unknown }).matched; if (typeof matched !== 'number' || !Number.isInteger(matched) || matched < 0) { - this.logger.warn?.( + this.logger?.warn?.( '[webhook-auto-enqueuer] dropping off-contract bulk data event: payload is not a ' + 'BulkDataEvent (no top-level non-negative integer `matched`) — fix the producer', { type: event.type, object: event.object }, @@ -912,7 +935,7 @@ export class AutoEnqueuer { // ever conflating two distinct ones. const eventUuid = (payload as { id?: unknown }).id; if (typeof eventUuid !== 'string' || eventUuid === '') { - this.logger.warn?.( + this.logger?.warn?.( '[webhook-auto-enqueuer] dropping off-contract bulk data event: payload has no ' + 'top-level string `id` to dedup on — fix the producer', { type: event.type, object: event.object }, @@ -958,7 +981,7 @@ export class AutoEnqueuer { // the admin just turned off. if (!event.type?.startsWith('data.record.') && !event.type?.startsWith('data.records.')) return; this.refresh().catch((err) => - this.logger.warn?.('[webhook-auto-enqueuer] self-heal refresh failed', err), + this.logger?.warn?.('[webhook-auto-enqueuer] self-heal refresh failed', err), ); } diff --git a/packages/services/service-knowledge/src/knowledge-service.ts b/packages/services/service-knowledge/src/knowledge-service.ts index 060aab2b44..b199cba8cd 100644 --- a/packages/services/service-knowledge/src/knowledge-service.ts +++ b/packages/services/service-knowledge/src/knowledge-service.ts @@ -21,7 +21,15 @@ import type { */ export interface KnowledgeLogger { info?(msg: string, ...rest: unknown[]): void; - warn?(msg: string, ...rest: unknown[]): void; + /** + * The GUARANTEED fallback channel (#9754). `error` stays optional — hosts do + * inject reduced sinks — so `warn` is where a durability report lands when + * `error` is absent, and a fallback that may itself be missing is not a + * fallback. Call sites keep the `logger?.warn?.(…)` spelling as the backstop + * for hosts the TYPE cannot reach; `SweepLogger` in plugin-email's + * `outbox-sweep.ts` carries the full reasoning and the measurement. + */ + warn(msg: string, ...rest: unknown[]): void; error?(msg: string, ...rest: unknown[]): void; debug?(msg: string, ...rest: unknown[]): void; } diff --git a/scripts/optional-error-sink-contract.baseline.json b/scripts/optional-error-sink-contract.baseline.json index d6ac704cdd..568bfc54f3 100644 --- a/scripts/optional-error-sink-contract.baseline.json +++ b/scripts/optional-error-sink-contract.baseline.json @@ -10,86 +10,29 @@ "", "⛔ This file is NOT an exemption list and NOT a place to add new work. A newly written sink", "with an optional `error` is a violation to fix, never a row to append. Every entry below", - "names WHY it is still here; what closes it is the same one-line repair in all but three", - "cases: drop the `?` from `warn`.", + "names WHY it is still here.", "", - "The 15 entries are the measured first-run population, not an estimate: 36 sink types in", - "packages/** declare `error`, 11 declare it REQUIRED, 10 declare it optional beside a", - "REQUIRED `warn` (the convention this repo already half-holds), and these 15 are the drift.", - "The PR that introduced the gate repaired the two the card names — `SweepLogger`", - "(plugin-email) and `ProjectionLogger` (plugin-security) — and recorded the rest here rather", - "than changing 13 more packages' exported contracts in one diff." + "HISTORY, so the shrink is auditable rather than asserted:", + " 17 red — the measured first-run population (36 sink types in packages/** declare `error`;", + " 11 declare it REQUIRED; 8 declared it optional beside a REQUIRED `warn`).", + " -2 — #9754's own PR repaired `SweepLogger` (plugin-email) and `ProjectionLogger`", + " (plugin-security), and recorded the remaining 15 here.", + " -12 — #10556 paid down twelve of the thirteen mechanical repairs: cloud-connection,", + " metadata-protocol, plugin-approvals/lifecycle-hooks, both plugin-audit sinks,", + " both plugin-auth sinks, plugin-email/attachment-reclaim, plugin-reports,", + " plugin-sharing, plugin-webhooks and service-knowledge.", + " = 3 — what is left, and NONE of the three is left for lack of effort:", + " one is a serialisation leftover whose reason has expired (approval-service.ts),", + " and two are DESIGN calls escalated to the maintainer rather than answered by a", + " dev to make this checker green (security-plugin.ts, settings-service.types.ts)." ], "entries": [ - { - "file": "packages/cloud-connection/src/cloud-connection-plugin.ts", - "sink": "logger@PluginContext", - "verdict": "optional-fallback", - "members": "{ info? warn? error? }", - "note": "Boot-time cloud-connection sink. Not repaired here: the value comes from the host kernel's `PluginContext`, so making `warn` required is a change to what every embedder must pass — sized separately from #9754's own surface." - }, - { - "file": "packages/metadata-protocol/src/migrations/partial-index-probe.ts", - "sink": "IndexMigrationLogger", - "verdict": "optional-fallback", - "members": "{ info? warn? error? }", - "note": "Partial-index migration probe. Mechanical to repair (`warn?` -> `warn`) once its callers are re-checked; left out of #9754's PR to keep the contract change to the two sinks the card names." - }, { "file": "packages/plugins/plugin-approvals/src/approval-service.ts", "sink": "logger@ApprovalServiceOptions", "verdict": "optional-fallback", "members": "{ info? warn? error? debug? }", - "note": "Approval-service options bag. Same mechanical repair, different package; not in #9754's named surface." - }, - { - "file": "packages/plugins/plugin-approvals/src/lifecycle-hooks.ts", - "sink": "MinimalLogger", - "verdict": "optional-fallback", - "members": "{ debug? info? warn? error? }", - "note": "Approval lifecycle hooks. Same mechanical repair, different package; not in #9754's named surface." - }, - { - "file": "packages/plugins/plugin-audit/src/auth-event-audit.ts", - "sink": "AuthEventAuditLogger", - "verdict": "optional-fallback", - "members": "{ error? warn? debug? }", - "note": "The sink #9754's body calls the sharpest instance — it declared `error?` and `debug?` and NO `warn` until #9750 added `warn?`. Deliberately NOT repaired here: `packages/plugins/plugin-audit` is the file surface of open PR #10450, and a contract change racing an open PR on the same package is how two correct diffs land jointly wrong." - }, - { - "file": "packages/plugins/plugin-audit/src/read-audit.ts", - "sink": "ReadAuditLogger", - "verdict": "optional-fallback", - "members": "{ error? warn? debug? }", - "note": "Sibling of `AuthEventAuditLogger` in the same package, and held for the same reason: `packages/plugins/plugin-audit` belongs to open PR #10450 while #9754 is in flight." - }, - { - "file": "packages/plugins/plugin-auth/src/member-role-canonical.ts", - "sink": "LoggerLike", - "verdict": "optional-fallback", - "members": "{ info? warn? error? debug? }", - "note": "Member-role canonicalisation sink. Same mechanical repair, different package; not in #9754's named surface." - }, - { - "file": "packages/plugins/plugin-auth/src/reconcile-membership.ts", - "sink": "logger@ReconcileMembershipDeps", - "verdict": "optional-fallback", - "members": "{ info? warn? error? }", - "note": "Membership reconciliation deps. Same mechanical repair, different package; not in #9754's named surface." - }, - { - "file": "packages/plugins/plugin-email/src/attachment-reclaim.ts", - "sink": "ReclaimLogger", - "verdict": "optional-fallback", - "members": "{ info? warn? error? }", - "note": "Attachment-reclaim sink, in the SAME package as the repaired `SweepLogger` and mechanically identical. Left for a follow-up on purpose: its call sites are a separate set from the sweep's, and #9754's PR keeps its blast radius to the two sinks whose consumers the lane PM had already measured." - }, - { - "file": "packages/plugins/plugin-reports/src/report-service.ts", - "sink": "logger@ReportServiceOptions", - "verdict": "optional-fallback", - "members": "{ info? warn? error? }", - "note": "Report-service options bag. Same mechanical repair, different package; not in #9754's named surface." + "note": "Approval-service options bag. Mechanically identical to the twelve repaired in #10556 — drop the `?` from `warn`. Held out of that PR to serialize against PR #10546, which owned this file while it was open. #10546 has since MERGED (2026-08-21), so the reason this row is still here has expired: it is a one-line repair awaiting a dispatch, not a design call and not an exemption." }, { "file": "packages/plugins/plugin-security/src/security-plugin.ts", @@ -98,27 +41,6 @@ "members": "{ info? warn? error? }", "note": "⚠️ NOT mechanical, and the sharpest live illustration of this rule: the field is initialised `= {}`, so today the plugin's own default sink prints nothing at all. Making `warn` required forces a DECISION about what that default should be (a console-backed sink, or a declared silent one), which is a design call rather than a `?`-deletion." }, - { - "file": "packages/plugins/plugin-sharing/src/bulk-recompute.ts", - "sink": "MinimalLogger", - "verdict": "optional-fallback", - "members": "{ info? warn? error? }", - "note": "Sharing bulk-recompute sink. Same mechanical repair, different package; not in #9754's named surface." - }, - { - "file": "packages/plugins/plugin-webhooks/src/auto-enqueuer.ts", - "sink": "OptionalLogger", - "verdict": "optional-fallback", - "members": "{ info? warn? debug? error? }", - "note": "Webhook auto-enqueuer sink. Same mechanical repair, different package; not in #9754's named surface." - }, - { - "file": "packages/services/service-knowledge/src/knowledge-service.ts", - "sink": "KnowledgeLogger", - "verdict": "optional-fallback", - "members": "{ info? warn? error? debug? }", - "note": "Knowledge-service sink. Its plugin builds the value by forwarding `ctx.logger` through a cast, so the repair is mechanical but touches the forwarding shape too; not in #9754's named surface." - }, { "file": "packages/services/service-settings/src/settings-service.types.ts", "sink": "SettingsDiagnosticsLogger",