Skip to content

Commit e222a53

Browse files
os-warrenclaude
andauthored
fix(plugins,services): guarantee a warn channel on twelve optional-error sinks (#10691)
* fix(plugins,services): guarantee a `warn` channel on twelve optional-`error` sinks Pays down twelve of the fifteen rows in the shrink-only optional-error sink ledger (#9754): each sink declaring an optional `error` now declares a NON-optional `warn`, so a durability report always has somewhere to land. `error` stays optional everywhere — requiring it was measured and rejected as #9754 option C. The twelve ledger rows are deleted in this same commit, which the checker requires (it fails on a stale entry). Three forwarding seams were tightened with them, each reported by `tsc` rather than guessed at: `MinimalLogger` in plugin-sharing's `rule-hooks.ts` and `record-share-cascade.ts` (both forward into bulk-recompute's now-guaranteed sink), and `AuthManagerConfig['logger']` in plugin-auth (forwards verbatim into `ReconcileMembershipDeps`). Two of the twelve turned out NOT to be one-line repairs: `report-service.ts` and `auto-enqueuer.ts` both defaulted their logger to `?? {}` — the same shape the ledger fences off as an open design call for plugin-security. Neither default was decided here. The field is now honestly optional instead of being given a minted sink that declares it can warn and discards everything; runtime behaviour is unchanged in both directions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx * fix(metadata-protocol): keep the lying-host backstop pin under the tightened sink `partial-index-probe.test.ts` passed `{}` as an `IndexMigrationLogger` to pin that `logProblem` degrades to silence rather than throwing. `{}` stopped being a legal value of that type when `warn` became non-optional, so the case now casts deliberately and says why: the type-level guarantee and the runtime backstop are different promises, and this case asserts the second. Caught by `check:type-check-debt --re-measure` (DEBT 63 -> 64), not by the package build: `@objectstack/metadata-protocol` declares no `typecheck` script, so its test files are hidden from tsc and a `--filter ... typecheck` run would have reported a silent zero-match pass. Ledger number untouched, back at 63. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx * test(plugin-auth): declare `warn` on three reconcile-membership logger doubles `ReconcileMembershipDeps['logger']` guarantees a `warn` channel (#9754), so the three `{ error: vi.fn() }` doubles in `reconcile-membership.test.ts` stopped being legal values of it. A fourth double in the same file already declared `warn` and was unaffected. Measured rather than assumed, because these files are HIDDEN from `pnpm typecheck` (plugin-auth's tsconfig excludes its own tests, TEST_DEBT): `tsc` over a tsconfig with the test exclusion dropped reports 98 with this branch's plugin-auth changes reverted and 101 with them, so the +3 was this branch's. With these three doubles repaired it is 98 again. ⛔ The TEST_DEBT ledger number is untouched at 109 and `--lower` was NOT run. The 11-error surplus #10615 reports is left exactly as that card describes it — neither consumed by this branch nor closed by it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx * fix(changeset): correct the type name, declare the break, and bump `minor` Three defects in the changeset text, all found in PM review of #10691: 1. `AuthManagerConfig` does not exist. The type is `AuthManagerOptions` (auth-manager.ts:375, public via `export * from './auth-manager.js'`), and the tightened `logger` field is :647 inside it. Measured: `AuthManagerConfig` has ZERO hits in `packages/**` source and only two in a stale CHANGELOG.md — so an embedder who hit the compile error and grepped the name this changeset shipped would have found an already-retired spelling and been led away from the fix. That is what makes it worse than a typo: a changeset body IS the published CHANGELOG. 2. `patch` contradicted the changeset's own text. `check-changeset-no-major.mjs` records the convention in its own WHY THIS EXISTS block: "During the launch window we ship breaking changes as `minor` (pre-1.0 semantics ...)" — every publishable package is in the Changesets `fixed` group and versions in lockstep, so the bump level is the only channel that tells the stack a release can break a build. A changeset that says "an external embedder ... will now see a compile error" cannot also be `patch`. Now `minor`; `major` is banned by that same guard. 3. The break is now DECLARED (`**BREAKING**`) and carries its ADR-0087 disposition, which a declared-breaking changeset must state in writing (AGENTS.md, enforced by `check:adr-0087-registration`). The marker is `runtime-interface-only`, naming the seven exported symbols, and it was chosen by reading the gate rather than by pattern-matching the common case: `no-migration-prescription` is REFUSED when the body carries a prescription, and `runtime-interface-only` inherits that same refusal (#8299) — it is a narrowing, never an escape. It is claimable here because it is TRUE and mechanically re-checkable: every tightened type is a plain TS logger interface, no Zod projection, no metadata surface and unreferenced by one, so `objectstack migrate meta` provably has nothing to rewrite. Nothing is removed or renamed, so AGENTS.md's FROM → TO requirement (scoped to removals and renames) is not triggered; the consumer action is stated plainly instead. Changeset text only — no source file is touched by this commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 4bafdde commit e222a53

21 files changed

Lines changed: 322 additions & 152 deletions

File tree

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
---
2+
"@objectstack/cloud-connection": minor
3+
"@objectstack/metadata-protocol": minor
4+
"@objectstack/plugin-approvals": minor
5+
"@objectstack/plugin-audit": minor
6+
"@objectstack/plugin-auth": minor
7+
"@objectstack/plugin-email": minor
8+
"@objectstack/plugin-reports": minor
9+
"@objectstack/plugin-sharing": minor
10+
"@objectstack/plugin-webhooks": minor
11+
"@objectstack/service-knowledge": minor
12+
---
13+
14+
**BREAKING** (compile-time only): twelve logger sink types that declared an
15+
optional `error` now declare a **non-optional** `warn`, so a durability report
16+
always has somewhere to land (#9754, #10556).
17+
18+
`minor`, not `major`: during the launch window this stack ships breaking changes
19+
as `minor` — every publishable package versions in lockstep, so a `major` would
20+
promote the whole release. `patch` would be wrong in the other direction, because
21+
this *can* break a consumer's build.
22+
23+
`error` stays optional on every one of these types — hosts legitimately inject
24+
reduced sinks, and requiring `error` was measured and rejected as #9754 option C.
25+
What changes is that its *absence* now has a declared, guaranteed destination.
26+
Call sites keep the `logger?.warn?.(…)` spelling as the backstop for hosts the
27+
type cannot reach, so **no runtime behaviour changes**: nothing that printed
28+
before stops printing, and nothing silent starts printing.
29+
30+
### Who has to change, and what to do
31+
32+
Only a caller that hands one of these sinks an object with **no `warn` method**
33+
for example `{ info }` or `{ error }` alone. Add a `warn` member; there is no
34+
rename, no removal, and no stored value or metadata key to rewrite. Every
35+
construction site inside this repo already supplied one, so the in-repo cost was
36+
zero; the compile error is reserved for the callers that were silently discarding
37+
these reports.
38+
39+
The affected types, by package:
40+
41+
- `@objectstack/cloud-connection` — the internal `PluginContext['logger']`
42+
- `@objectstack/metadata-protocol``IndexMigrationLogger`
43+
- `@objectstack/plugin-approvals` — the internal `MinimalLogger` of `lifecycle-hooks`
44+
- `@objectstack/plugin-audit``AuthEventAuditLogger`, `ReadAuditLogger`
45+
- `@objectstack/plugin-auth``ReconcileMembershipDeps['logger']`, the internal
46+
`LoggerLike` of `member-role-canonical`, and `AuthManagerOptions['logger']`
47+
- `@objectstack/plugin-email``ReclaimLogger`, via `ReclaimAttachmentContentOptions`
48+
- `@objectstack/plugin-reports``ReportServiceOptions['logger']`
49+
- `@objectstack/plugin-sharing` — the internal `MinimalLogger` of `bulk-recompute`,
50+
`rule-hooks` and `record-share-cascade`
51+
- `@objectstack/plugin-webhooks``OptionalLogger`, via `AutoEnqueuerOptions`
52+
- `@objectstack/service-knowledge``KnowledgeLogger`
53+
54+
`AuthManagerOptions['logger']` is the one most likely to be reached from outside:
55+
`AuthManager` is public surface, its `logger` option stays optional, and a logger
56+
that *is* supplied must now carry `warn`. The only non-test construction site in
57+
this repo passes the kernel `Logger`, whose `warn` is already required.
58+
59+
`ReportService` and `AutoEnqueuer` additionally stopped defaulting their logger
60+
field to `{}`. The field is now honestly optional rather than holding an empty
61+
object that declared it could report and discarded everything. Behaviour is
62+
unchanged in both directions.
63+
64+
<!-- adr-0087: not-required (runtime-interface-only packages/plugins/plugin-auth/src/auth-manager.ts#AuthManagerOptions, packages/plugins/plugin-auth/src/reconcile-membership.ts#ReconcileMembershipDeps, packages/metadata-protocol/src/migrations/partial-index-probe.ts#IndexMigrationLogger, packages/plugins/plugin-audit/src/auth-event-audit.ts#AuthEventAuditLogger, packages/plugins/plugin-audit/src/read-audit.ts#ReadAuditLogger, packages/plugins/plugin-reports/src/report-service.ts#ReportServiceOptions, packages/services/service-knowledge/src/knowledge-service.ts#KnowledgeLogger) every tightened type is a plain TypeScript logger interface -- no Zod projection, no metadata surface, and none is referenced by one -- so `objectstack migrate meta` has nothing to rewrite. Nothing is removed or renamed and no stored value moves; the only consumer action is adding a `warn` member at a construction site the compiler names. -->

packages/cloud-connection/src/cloud-connection-plugin.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,15 @@ interface PluginContext {
5656
getService<T = any>(name: string): T;
5757
logger?: {
5858
info?: (msg: string) => void;
59-
warn?: (msg: string) => void;
59+
/**
60+
* The GUARANTEED fallback channel (#9754). `error` stays optional — hosts do
61+
* inject reduced sinks — so `warn` is where a durability report lands when
62+
* `error` is absent, and a fallback that may itself be missing is not a
63+
* fallback. Call sites keep the `logger?.warn?.(…)` spelling as the backstop
64+
* for hosts the TYPE cannot reach; `SweepLogger` in plugin-email's
65+
* `outbox-sweep.ts` carries the full reasoning and the measurement.
66+
*/
67+
warn: (msg: string) => void;
6068
error?: (msg: string, err?: unknown) => void;
6169
};
6270
}

packages/metadata-protocol/src/migrations/partial-index-probe.test.ts

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
probeThenReplaceIndex,
1212
type IndexExec,
1313
} from './partial-index-probe.js';
14+
import type { IndexMigrationLogger } from './partial-index-probe.js';
1415

1516
/**
1617
* The probe-first order, tested where it lives (#6418).
@@ -382,6 +383,18 @@ describe('probe-first partial index replacement (#6418)', () => {
382383
expect(warnOnly.warn).toHaveBeenCalledWith('msg', { detail: 'detail' });
383384

384385
expect(() => logProblem(undefined, 'msg', 'detail')).not.toThrow();
385-
expect(() => logProblem({}, 'msg', 'detail')).not.toThrow();
386+
// `{}` is no longer a legal `IndexMigrationLogger` — #9754 made `warn`
387+
// non-optional precisely so a sink with NEITHER channel cannot be
388+
// written. The cast is deliberate and is the point of the case: it
389+
// forces through the one host the TYPE cannot reach (a plain-JS
390+
// embedder, or a cast at the boundary) and pins that `logProblem`'s
391+
// `?.` backstop still degrades to silence instead of throwing
392+
// `logger.warn is not a function` inside a migration probe. Type-level
393+
// guarantee and runtime backstop are different promises; this asserts
394+
// the second one, and the compile error that used to be impossible here
395+
// is now what proves the first.
396+
expect(() =>
397+
logProblem({} as unknown as IndexMigrationLogger, 'msg', 'detail'),
398+
).not.toThrow();
386399
});
387400
});

packages/metadata-protocol/src/migrations/partial-index-probe.ts

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -97,13 +97,24 @@ export function resolveIndexExecForTable(engine: unknown, table: string): IndexE
9797

9898
/**
9999
* Minimal logger surface, structurally compatible with `@objectstack/spec`'s
100-
* `Logger` (every method optional so a bare console or a test double fits).
100+
* `Logger` (a bare console or a test double fits — but see `warn` below, which
101+
* #9754 made non-optional, so a double must now declare it).
101102
* Signatures mirror that contract exactly — notably `error(msg, Error, meta)`
102103
* versus `warn(msg, meta)` — so a host `Logger` is assignable as-is.
103104
*/
104105
export interface IndexMigrationLogger {
105106
info?(message: string, meta?: Record<string, any>): void;
106-
warn?(message: string, meta?: Record<string, any>): void;
107+
/**
108+
* The GUARANTEED fallback channel (#9754). `error` stays optional — hosts do
109+
* inject reduced sinks — so `warn` is where a durability report lands when
110+
* `error` is absent, and a fallback that may itself be missing is not a
111+
* fallback. `logProblem` below is exactly that degrade, and until this member
112+
* was required it could reach for two channels and find neither. Call sites
113+
* keep the `logger?.warn?.(…)` spelling as the backstop for hosts the TYPE
114+
* cannot reach; `SweepLogger` in plugin-email's `outbox-sweep.ts` carries the
115+
* full reasoning and the measurement.
116+
*/
117+
warn(message: string, meta?: Record<string, any>): void;
107118
error?(message: string, error?: Error, meta?: Record<string, any>): void;
108119
}
109120

packages/plugins/plugin-approvals/src/lifecycle-hooks.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,15 @@ interface MinimalEngine {
8787
interface MinimalLogger {
8888
debug?: (msg: any, ...rest: any[]) => void;
8989
info?: (msg: any, ...rest: any[]) => void;
90-
warn?: (msg: any, ...rest: any[]) => void;
90+
/**
91+
* The GUARANTEED fallback channel (#9754). `error` stays optional — hosts do
92+
* inject reduced sinks — so `warn` is where a durability report lands when
93+
* `error` is absent, and a fallback that may itself be missing is not a
94+
* fallback. Call sites keep the `logger?.warn?.(…)` spelling as the backstop
95+
* for hosts the TYPE cannot reach; `SweepLogger` in plugin-email's
96+
* `outbox-sweep.ts` carries the full reasoning and the measurement.
97+
*/
98+
warn: (msg: any, ...rest: any[]) => void;
9199
error?: (msg: any, ...rest: any[]) => void;
92100
}
93101

packages/plugins/plugin-audit/src/auth-event-audit.test.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -304,13 +304,20 @@ describe('[#8144] createAuthEventAuditSink writes a login row that names its act
304304
throw new Error('no such table: sys_audit_log');
305305
},
306306
};
307-
const logger = { error: vi.fn(), debug: vi.fn() };
307+
// `warn` is required by `AuthEventAuditLogger` (#9754/#10556): a sink that
308+
// declares an optional `error` must be able to degrade. Present here AND
309+
// asserted unused below, which pins the ORDER — `error` first, `warn` only
310+
// as the fallback. Before the contract change this double was `{ error,
311+
// debug }`, a shape the type accepted and the degrade path could not use.
312+
const logger = { error: vi.fn(), warn: vi.fn(), debug: vi.fn() };
308313
const sink = createAuthEventAuditSink({ getEngine: () => broken, logger });
309314

310315
await expect(sink.recordAuthEvent({ action: 'login', userId: 'usr_1' })).resolves.toBeUndefined();
311316
await sink.recordAuthEvent({ action: 'logout', userId: 'usr_1' });
312317

313318
expect(logger.error).toHaveBeenCalledTimes(1);
319+
// The fallback stays untouched while `error` exists.
320+
expect(logger.warn).not.toHaveBeenCalled();
314321
const [msg] = logger.error.mock.calls[0];
315322
// The two things a durability `error` owes, in its first line.
316323
expect(String(msg)).toContain('INCOMPLETE');

packages/plugins/plugin-audit/src/auth-event-audit.ts

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -106,13 +106,19 @@ export interface AuthSessionAuditEvent {
106106
export interface AuthEventAuditLogger {
107107
error?(msg: string, err?: Error, meta?: Record<string, any>): void;
108108
/**
109-
* The fallback channel for the durability report below. `error` is optional
110-
* here, so a sink that has none must still have somewhere to put a lost audit
111-
* row — reaching for `error` and finding nothing must degrade to `warn`,
112-
* never to silence (#9657). Signature and optionality mirror
113-
* `ReadAuditLogger` in `read-audit.ts`, which already declared it.
109+
* The GUARANTEED fallback channel for the durability report below. `error` is
110+
* optional here, so a sink that has none must still have somewhere to put a
111+
* lost audit row — reaching for `error` and finding nothing must degrade to
112+
* `warn`, never to silence (#9657).
113+
*
114+
* This is the sink #9754's body calls the sharpest instance: it declared
115+
* `error?` and `debug?` and NO `warn` at all, so the call site below COULD NOT
116+
* have been written correctly against the contract it was given. #9750 added
117+
* `warn?`, which gave it something to reach for and still no guarantee it was
118+
* there; non-optional (#9754) is what makes the silence unrepresentable.
119+
* Signature and optionality mirror `ReadAuditLogger` in `read-audit.ts`.
114120
*/
115-
warn?(msg: string, meta?: Record<string, any>): void;
121+
warn(msg: string, meta?: Record<string, any>): void;
116122
debug?(msg: string, meta?: Record<string, any>): void;
117123
}
118124

packages/plugins/plugin-audit/src/read-audit.test.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -314,10 +314,18 @@ describe('#8992 the write is OFF the request path', () => {
314314
it('a ledger write failure never reaches the read, and reports once at `error`', async () => {
315315
const errors: string[] = [];
316316
const debugs: string[] = [];
317+
// Required by `ReadAuditLogger` (#9754/#10556) — and asserted unused below,
318+
// so the double pins that `error` is reached for FIRST and `warn` is only
319+
// the degrade path.
320+
const warns: string[] = [];
317321
const writer = installReadAuditWriter(engine, {
318322
objects: ['contact'],
319323
timers: makeManualTimers(),
320-
logger: { error: (m: string) => errors.push(m), debug: (m: string) => debugs.push(m) },
324+
logger: {
325+
error: (m: string) => errors.push(m),
326+
warn: (m: string) => warns.push(m),
327+
debug: (m: string) => debugs.push(m),
328+
},
321329
})!;
322330
// Break the ledger AFTER install, so the probe has already run.
323331
(engine as any).insert = async () => { throw new Error('no such table: sys_audit_log'); };

packages/plugins/plugin-audit/src/read-audit.ts

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,15 @@ export const READ_AUDIT_ACTION = 'read';
112112
/** Minimal logger surface — structurally the kernel `ctx.logger` (`ILogger`). */
113113
export interface ReadAuditLogger {
114114
error?(msg: string, err?: Error, meta?: Record<string, any>): void;
115-
warn?(msg: string, meta?: Record<string, any>): void;
115+
/**
116+
* The GUARANTEED fallback channel (#9754). `error` stays optional — hosts do
117+
* inject reduced sinks — so `warn` is where a durability report lands when
118+
* `error` is absent, and a fallback that may itself be missing is not a
119+
* fallback. Call sites keep the `logger?.warn?.(…)` spelling as the backstop
120+
* for hosts the TYPE cannot reach; `SweepLogger` in plugin-email's
121+
* `outbox-sweep.ts` carries the full reasoning and the measurement.
122+
*/
123+
warn(msg: string, meta?: Record<string, any>): void;
116124
debug?(msg: string, meta?: Record<string, any>): void;
117125
}
118126

packages/plugins/plugin-auth/src/auth-manager.ts

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -630,8 +630,20 @@ export interface AuthManagerOptions extends Partial<AuthConfig> {
630630
* Optional structured logger (the kernel `ctx.logger`) for best-effort
631631
* bookkeeping surfaces such as the ADR-0093 membership reconciler. Omitted →
632632
* those surfaces run silently (they already fail closed to no-op).
633+
*
634+
* The whole field stays OPTIONAL — omitting it is still a supported posture.
635+
* What is no longer representable is supplying a logger that cannot carry a
636+
* durability report: `warn` is non-optional because this value is FORWARDED
637+
* verbatim into `ReconcileMembershipDeps.logger` (reconcile-membership.ts),
638+
* whose sink guarantees that channel under #9754. Measured before tightening:
639+
* the only non-test construction site in this repo is `auth-plugin.ts`, which
640+
* passes `ctx.logger` — the kernel `Logger`, whose `warn` is already
641+
* required — so the in-tree cost is zero. An external embedder handing
642+
* `AuthManager` a reduced `{ info }` sink is the one caller this asks to
643+
* change, and that is the point: it was the caller silently discarding the
644+
* reconciler's reports (#10556).
633645
*/
634-
logger?: { info?: (msg: string, meta?: any) => void; warn?: (msg: string, meta?: any) => void };
646+
logger?: { info?: (msg: string, meta?: any) => void; warn: (msg: string, meta?: any) => void };
635647

636648
/**
637649
* ADR-0069 D2 — account lockout (anti-brute-force). After this many

0 commit comments

Comments
 (0)