Skip to content

Commit e9534a4

Browse files
os-steveclaude
andauthored
fix(devx): durability log-level matcher reads the callee, and stops accepting a spelling that prints nothing (#9750)
* fix(devx): the durability log-level matcher reads the callee, and stops accepting a spelling that prints nothing `loggerLevel()` required the callee to be a plain property access, so `(logger.error ?? logger.warn)(...)` — a call on a parenthesized expression — collected no levels and the catch was reported as `catch swallows the failure with no log at all`, the harshest verdict in the file, on code that is loud at runtime. The dangerous half was the repair that report invited. The one fallback spelling the matcher DID accept is `logger.error?.(...)`, which prints nothing at all against a sink that has no `error` — and `error` is declared optional on exactly the sinks that use the idiom. So the gate's cheapest satisfaction converted a loud degradation into a silent one. - resolve the callee structurally: parentheses, `??`/`||`, a ternary, `.call`/`.apply`/`.bind`, non-null assertions, `logger['error']`, and a same-file `const` holding a fallback are followed to whatever they end at. - an optional CALL (`?.(`) no longer counts as loud: it is the author's own statement that the call may not print, and the sink it holds still has `warn`. Optionality on the RECEIVER (`logger?.error(...)`) is deliberately not judged. - "I could not read this call" is its own verdict, `unreadable-report`, instead of being folded into `silent-swallow`. - six seams the tightened rule found — plugin-audit x3, plugin-email, plugin-security x2, all on sinks whose `error` is declared optional — now reach for `error` and fall back to `warn` instead of to silence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XqDQYVU5smx29ts9pAErja * test(plugins): pin that a sink without `error` still hears the durability report One per repaired package. Each asserts the MESSAGE lands at `warn` — not merely that nothing threw, which the silent `logger?.error?.(…)` version satisfied too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XqDQYVU5smx29ts9pAErja * fix(plugin-audit): AuthEventAuditLogger declares the `warn` fallback its report needs `error` is optional on this sink, so a durability report that reaches for it needs somewhere to fall back to — and this interface declared no `warn` at all, while its sibling `ReadAuditLogger` in the same package always has. The type, not the call site, was the thing missing the channel. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XqDQYVU5smx29ts9pAErja * chore: changeset for the durability-report fallback, and re-measure the shape census Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XqDQYVU5smx29ts9pAErja --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent bcf2755 commit e9534a4

11 files changed

Lines changed: 806 additions & 77 deletions
Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
---
2+
"@objectstack/plugin-audit": patch
3+
"@objectstack/plugin-email": patch
4+
"@objectstack/plugin-security": patch
5+
---
6+
7+
A durability failure reported to a logger without `error` is no longer lost
8+
9+
Six degradation reports — a lost `sys_audit_log` row (CRUD, auth-event and
10+
read-audit writers), a stranded `sys_email` row, and the two permission-set
11+
metadata backfill failures — were spelled `logger?.error?.(…)`. `error` is
12+
declared OPTIONAL on those sinks, and an optional call emits nothing at all when
13+
the method is absent: a host injecting a `{ info, warn }` logger received no
14+
report whatsoever, on exactly the paths whose whole point is that nothing else
15+
looks broken afterwards.
16+
17+
Each now reaches for `error` and falls back to `warn`, never to silence. The
18+
message, its consequence and its fix are identical on both channels; only the
19+
level degrades, and only when the sink cannot do better.
20+
21+
`AuthEventAuditLogger` additionally declares the `warn?` method it needs for
22+
that fallback, matching `ReadAuditLogger`, which always had it. The addition is
23+
optional, so no existing sink stops satisfying the interface.

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

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1068,7 +1068,7 @@ describe('audit writers — a lost audit row is reported at error (#5226)', () =
10681068
interface LogLine { level: string; message: string; meta?: any }
10691069

10701070
/** Engine whose `sys_audit_log` insert always fails, capturing every log line. */
1071-
function makeFailingEngine(failWith = 'no such table: sys_audit_log') {
1071+
function makeFailingEngine(failWith = 'no such table: sys_audit_log', omitError = false) {
10721072
const hooks = new Map<string, Array<(ctx: any) => any>>();
10731073
const logs: LogLine[] = [];
10741074
const sudoApi = {
@@ -1095,7 +1095,12 @@ describe('audit writers — a lost audit row is reported at error (#5226)', () =
10951095
},
10961096
unregisterHooksByPackage() { /* no-op */ },
10971097
logger: {
1098-
error(message: string, _err?: unknown, meta?: any) { logs.push({ level: 'error', message, meta }); },
1098+
// `omitError` reproduces the sink a host may legitimately inject: the
1099+
// kernel `Logger` requires `error`, but this reporter reaches its sink
1100+
// through `(engine as any).logger`, so nothing checks. #9657.
1101+
...(omitError
1102+
? {}
1103+
: { error(message: string, _err?: unknown, meta?: any) { logs.push({ level: 'error', message, meta }); } }),
10991104
warn(message: string, meta?: any) { logs.push({ level: 'warn', message, meta }); },
11001105
debug(message: string, meta?: any) { logs.push({ level: 'debug', message, meta }); },
11011106
info() { /* unused */ },
@@ -1127,6 +1132,24 @@ describe('audit writers — a lost audit row is reported at error (#5226)', () =
11271132
expect(errors[0].meta).toMatchObject({ object: 'crm_lead', action: 'create' });
11281133
});
11291134

1135+
it('still reports the lost row when the sink has NO `error` — at warn, never in silence (#9657)', async () => {
1136+
// The regression this pins: the report used to be spelled
1137+
// `logger?.error?.(…)`, an optional call that emits NOTHING against a sink
1138+
// without `error`. The compliance trail was then incomplete AND unreported.
1139+
// ⛔ Asserting only "did not throw" would pass on the silent version too,
1140+
// so this asserts the MESSAGE lands, and that it is the same one.
1141+
const { engine, fire, logs } = makeFailingEngine('no such table: sys_audit_log', true);
1142+
installAuditWriters(engine as any);
1143+
1144+
await fire('afterInsert', aWrite('l-1'));
1145+
1146+
const warns = logs.filter((l) => l.level === 'warn');
1147+
expect(warns).toHaveLength(1);
1148+
expect(warns[0].message).toMatch(/compliance trail is now INCOMPLETE/);
1149+
expect(warns[0].message).toMatch(/OS_TELEMETRY_DB=0/);
1150+
expect(warns[0].meta).toMatchObject({ object: 'crm_lead', action: 'create' });
1151+
});
1152+
11301153
it('names both the CONSEQUENCE and the FIX in the first line it prints', async () => {
11311154
const { engine, fire, logs } = makeFailingEngine();
11321155
installAuditWriters(engine as any);

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

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -881,7 +881,7 @@ export function installAuditWriters(
881881
auditFailureReported = true;
882882
// The two things an `error` here owes, both in the first line it prints:
883883
// the CONSEQUENCE, concretely, and the FIX.
884-
logger?.error?.(
884+
const message =
885885
'Audit write FAILED — the compliance trail is now INCOMPLETE. The audited write itself SUCCEEDED and is on ' +
886886
'disk, so the API returned success and nothing downstream looks broken; only the `sys_audit_log` row that ' +
887887
'records who did it never landed, and nothing retries it. Every subsequent audited write is likely losing ' +
@@ -890,10 +890,16 @@ export function installAuditWriters(
890890
"lifecycle class routes it to the dedicated `telemetry` datasource whenever one is registered (`os dev` " +
891891
'provisions one by default as a SIBLING SQLite file), so a "no such table" here usually means the write ' +
892892
'executed against a DIFFERENT datasource than the one the table was created in — see framework#5226. ' +
893-
'Set `OS_TELEMETRY_DB=0` to keep every lifecycle-classed object on the primary datasource.',
894-
err instanceof Error ? err : new Error(detail),
895-
{ object, action },
896-
);
893+
'Set `OS_TELEMETRY_DB=0` to keep every lifecycle-classed object on the primary datasource.';
894+
// `error` is OPTIONAL on this sink, so `logger?.error?.(…)` printed
895+
// NOTHING when the host injected one without it — the durability
896+
// degradation this text describes would then be reported by nobody at
897+
// all (#9657). Reach for `error`, fall back to `warn`, never to silence.
898+
if (logger?.error) {
899+
logger.error(message, err instanceof Error ? err : new Error(detail), { object, action });
900+
} else {
901+
logger?.warn?.(message, { object, action, err: detail });
902+
}
897903
} catch {
898904
/* logging must never break the audited write */
899905
}

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -319,4 +319,27 @@ describe('[#8144] createAuthEventAuditSink writes a login row that names its act
319319
// on a systemic cause trains everyone to skim the channel.
320320
expect(logger.debug).toHaveBeenCalledTimes(1);
321321
});
322+
323+
it('[#9657] a sink with NO `error` still hears it — at warn, not in silence', async () => {
324+
// `AuthEventAuditLogger.error` is OPTIONAL and the report used to be
325+
// `logger?.error?.(…)`, an optional call that emits NOTHING when the method
326+
// is absent. The sign-in still succeeds either way, which is exactly why
327+
// the missing ledger row has to be somebody's problem out loud.
328+
const broken: any = {
329+
getSchema: () => null,
330+
insert: async () => {
331+
throw new Error('no such table: sys_audit_log');
332+
},
333+
};
334+
const logger = { warn: vi.fn(), debug: vi.fn() };
335+
const sink = createAuthEventAuditSink({ getEngine: () => broken, logger });
336+
337+
await expect(sink.recordAuthEvent({ action: 'login', userId: 'usr_1' })).resolves.toBeUndefined();
338+
339+
expect(logger.warn).toHaveBeenCalledTimes(1);
340+
const [msg, meta] = logger.warn.mock.calls[0];
341+
expect(String(msg)).toContain('INCOMPLETE');
342+
expect(String(msg)).toContain('Fix:');
343+
expect(meta).toMatchObject({ action: 'login' });
344+
});
322345
});

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

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,14 @@ export interface AuthSessionAuditEvent {
105105
*/
106106
export interface AuthEventAuditLogger {
107107
error?(msg: string, err?: Error, meta?: Record<string, any>): void;
108+
/**
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.
114+
*/
115+
warn?(msg: string, meta?: Record<string, any>): void;
108116
debug?(msg: string, meta?: Record<string, any>): void;
109117
}
110118

@@ -172,7 +180,7 @@ export function createAuthEventAuditSink(opts: AuthEventAuditSinkOptions): AuthE
172180
return;
173181
}
174182
failureReported = true;
175-
logger?.error?.(
183+
const message =
176184
'Auth-event audit write FAILED — the compliance trail is now INCOMPLETE. The sign-in/sign-out itself ' +
177185
'SUCCEEDED and the user holds a valid session, so the API returned 200 and nothing downstream looks ' +
178186
`broken; only the \`sys_audit_log\` row recording the ${action} never landed, and nothing retries it. ` +
@@ -183,10 +191,16 @@ export function createAuthEventAuditSink(opts: AuthEventAuditSinkOptions): AuthE
183191
'lifecycle class routes it to the dedicated `telemetry` datasource whenever one is registered (`os dev` ' +
184192
'provisions one by default as a SIBLING SQLite file), so a "no such table" here usually means the write ' +
185193
'executed against a DIFFERENT datasource than the one the table was created in. Set `OS_TELEMETRY_DB=0` ' +
186-
'to keep every lifecycle-classed object on the primary datasource.',
187-
err instanceof Error ? err : new Error(detail),
188-
{ action },
189-
);
194+
'to keep every lifecycle-classed object on the primary datasource.';
195+
// `error` is OPTIONAL on this sink, so `logger?.error?.(…)` printed
196+
// NOTHING when the host injected one without it — the durability
197+
// degradation this text describes would then be reported by nobody at
198+
// all (#9657). Reach for `error`, fall back to `warn`, never to silence.
199+
if (logger?.error) {
200+
logger.error(message, err instanceof Error ? err : new Error(detail), { action });
201+
} else {
202+
logger?.warn?.(message, { action, err: detail });
203+
}
190204
} catch {
191205
/* logging must never break the auth response */
192206
}

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

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -461,7 +461,7 @@ export function installReadAuditWriter(
461461
return;
462462
}
463463
failureReported = true;
464-
logger?.error?.(
464+
const message =
465465
`Read-audit write FAILED — ${count} record-view row(s) were LOST and the compliance trail is now ` +
466466
'INCOMPLETE. The reads themselves SUCCEEDED and returned 200, so the API, the screens and every ' +
467467
'counter read clean; only the `sys_audit_log` rows recording WHO opened those records never landed, ' +
@@ -473,10 +473,16 @@ export function installReadAuditWriter(
473473
'one is registered (`os dev` provisions one by default as a SIBLING SQLite file), so a "no such ' +
474474
'table" here usually means the write executed against a DIFFERENT datasource than the one the table ' +
475475
'was created in. Set `OS_TELEMETRY_DB=0` to keep every lifecycle-classed object on the primary ' +
476-
'datasource.',
477-
err instanceof Error ? err : new Error(detail),
478-
{ count },
479-
);
476+
'datasource.';
477+
// `error` is OPTIONAL on this sink, so `logger?.error?.(…)` printed
478+
// NOTHING when the host injected one without it — the durability
479+
// degradation this text describes would then be reported by nobody at
480+
// all (#9657). Reach for `error`, fall back to `warn`, never to silence.
481+
if (logger?.error) {
482+
logger.error(message, err instanceof Error ? err : new Error(detail), { count });
483+
} else {
484+
logger?.warn?.(message, { count, err: detail });
485+
}
480486
} catch {
481487
/* logging must never break the read */
482488
}

packages/plugins/plugin-email/src/outbox-sweep.test.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,26 @@ describe('failures are loud, counted, and never stop the batch', () => {
270270
expect(perRow[0]).toMatch(/engine exploded/);
271271
});
272272

273+
it('[#9657] reports a stranded row to a sink with NO `error` — at warn, not in silence', async () => {
274+
// `SweepLogger.error` is declared OPTIONAL, and the per-row report used to
275+
// be `logger?.error?.(…)`: against a `{ info, warn }` sink it emitted
276+
// NOTHING, so the message that stays `queued` forever was reported by
277+
// nobody while the server kept looking healthy.
278+
const engine = fakeEngine([{ id: 'row-bad', created_at: ago(min(30)) }]);
279+
const service = fakeService({
280+
deliver: () => { throw new Error('engine exploded'); },
281+
});
282+
const logger = { info: vi.fn(), warn: vi.fn() };
283+
284+
const res = await sweepStrandedOutbox({ engine, service, logger, now: () => NOW });
285+
286+
expect(res).toMatchObject({ scanned: 1, failed: 1 });
287+
const warned = lines(logger.warn).filter((l) => l.includes('could not advance sys_email row'));
288+
expect(warned).toHaveLength(1);
289+
expect(warned[0]).toMatch(/row-bad/);
290+
expect(warned[0]).toMatch(/engine exploded/); // the cause survives the fallback
291+
});
292+
273293
it('propagates a failure of the query itself — the sweep did not happen', async () => {
274294
const engine = { find: vi.fn(async () => { throw new Error('no such table: sys_email'); }) };
275295
await expect(sweepStrandedOutbox({ engine, service: fakeService(), now: () => NOW }))

packages/plugins/plugin-email/src/outbox-sweep.ts

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -199,13 +199,17 @@ export async function sweepStrandedOutbox(
199199
result.failed++;
200200
if (!rowErrorReported) {
201201
rowErrorReported = true;
202-
logger?.error?.(
202+
const message =
203203
`EmailServicePlugin: outbox sweep could not advance sys_email row '${rowId}' — that message stays `
204204
+ 'at `queued`, undelivered, and nothing will look at it again until the next restart, while the '
205205
+ 'server keeps reporting healthy. Fix: the cause below comes from the datasource or the queue, '
206206
+ 'not from the message itself (a message that cannot be sent is recorded as `failed` on its own '
207-
+ `row); restore that dependency and restart to re-sweep. Cause: ${err?.message ?? err}`,
208-
);
207+
+ `row); restore that dependency and restart to re-sweep. Cause: ${err?.message ?? err}`;
208+
// `SweepLogger.error` is OPTIONAL, so `logger?.error?.(…)` printed
209+
// NOTHING against a sink that has only `warn` — the stranded row would
210+
// then be reported by nobody (#9657). Fall back to `warn`, not silence.
211+
if (logger?.error) logger.error(message);
212+
else logger?.warn?.(message);
209213
}
210214
}
211215
}

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1016,6 +1016,36 @@ describe('reconcilePermissionSetProjection', () => {
10161016
expect(logs.some((l) => l.level === 'info' && /reconciled/.test(l.msg))).toBe(false);
10171017
});
10181018

1019+
it('[#9657] a sink with NO `error` still hears the backfill failure — at warn, not in silence', async () => {
1020+
// `ProjectionLogger.error` is declared OPTIONAL, and the report used to be
1021+
// spelled `logger?.error?.(…)` — an optional call that emits NOTHING when
1022+
// the method is absent. A host injecting `{ info, warn }` therefore lost
1023+
// the whole durability report, on the one path that must never be quiet.
1024+
const ql = makeQl();
1025+
const protocol = makeProtocol(ql);
1026+
ql.permRows.push({
1027+
id: 'ps_bad', name: 'broken_set', managed_by: 'admin', active: true,
1028+
label: 'Broken Set', object_permissions: JSON.stringify({ ticket: { allowRead: 'yes-please' } }),
1029+
});
1030+
const logs: Array<{ level: string; msg: string; meta?: any }> = [];
1031+
const logger = {
1032+
info: (m: string, meta?: any) => logs.push({ level: 'info', msg: m, meta }),
1033+
warn: (m: string, meta?: any) => logs.push({ level: 'warn', msg: m, meta }),
1034+
};
1035+
1036+
const out = await reconcilePermissionSetProjection(protocol, { ql, logger });
1037+
1038+
expect(out.backfillFailed).toBe(1);
1039+
const firstFailure = logs.find((l) => /backfill into metadata FAILED/.test(l.msg));
1040+
expect(firstFailure).toBeDefined();
1041+
expect(firstFailure!.level).toBe('warn');
1042+
// The consequence and the fix survive the fallback — a downgraded level is
1043+
// a degradation of the CHANNEL, never of the message.
1044+
expect(firstFailure!.msg).toMatch(/Nothing will look broken/);
1045+
expect(firstFailure!.msg).toMatch(/Fix:/);
1046+
expect(firstFailure!.meta?.name).toBe('broken_set');
1047+
});
1048+
10191049
it('heals a record that drifted from an EXISTING metadata definition (metadata wins)', async () => {
10201050
const ql = makeQl();
10211051
const declared = { member_default: envBody({ name: 'member_default', systemPermissions: ['declared.baseline'] }) };

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

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -800,14 +800,17 @@ export function createPermissionSetWriteThrough(
800800
// definition never returned to the metadata store — the stores
801801
// disagree silently until someone notices the set behaves like a
802802
// legacy data-door row.
803-
logger?.error?.(
803+
const message =
804804
'[security] restored permission set was NOT re-authored into metadata (ADR-0094 D3) — the record is ' +
805805
'back and looks healthy, but the metadata store has no definition for it, so a metadata-driven ' +
806806
're-provision will not recreate it. Fix: make the record body spec-valid (the error names the ' +
807-
'offending key) and re-save the set through Setup, or re-run boot reconciliation.',
808-
e as Error,
809-
{ name: row.name },
810-
);
807+
'offending key) and re-save the set through Setup, or re-run boot reconciliation.';
808+
// `ProjectionLogger.error` is OPTIONAL, so `logger?.error?.(…)` printed
809+
// NOTHING against a sink that has only `warn` — the durability
810+
// degradation described above would then be reported by nobody at all
811+
// (#9657). Reach for `error`, fall back to `warn`, never to silence.
812+
if (logger?.error) logger.error(message, e as Error, { name: row.name });
813+
else logger?.warn?.(message, { name: row.name, error: String((e as Error)?.message ?? e) });
811814
}
812815
}
813816
return;
@@ -1023,7 +1026,7 @@ export async function reconcilePermissionSetProjection(
10231026
// the summary line below. #4669: this was a `warn` with no counter,
10241027
// which is why a 100%-failing backfill sat green for a release.
10251028
if (out.backfillFailed === 1) {
1026-
logger?.error?.(
1029+
const message =
10271030
'[security] permission-set backfill into metadata FAILED (ADR-0094 D4) — this environment has ' +
10281031
'`sys_permission_set` records with NO metadata definition backing them, and the one-time backfill ' +
10291032
'did not write one. Nothing will look broken: the records still list in Setup and the evaluator ' +
@@ -1032,10 +1035,13 @@ export async function reconcilePermissionSetProjection(
10321035
'of them, and every boot retries and fails identically. Fix: make the record body spec-valid — the ' +
10331036
'error below names the offending key; `permissionSetBodyFromRow()` already drops storage columns ' +
10341037
'(`active`, timestamps, provenance), so a rejection here means the stored facet JSON itself is ' +
1035-
'off-contract — then reboot to re-run reconciliation, or delete the orphan record.',
1036-
e as Error,
1037-
{ name: row.name },
1038-
);
1038+
'off-contract — then reboot to re-run reconciliation, or delete the orphan record.';
1039+
// `ProjectionLogger.error` is OPTIONAL, so `logger?.error?.(…)` printed
1040+
// NOTHING against a sink that has only `warn` — the durability
1041+
// degradation described above would then be reported by nobody at all
1042+
// (#9657). Reach for `error`, fall back to `warn`, never to silence.
1043+
if (logger?.error) logger.error(message, e as Error, { name: row.name });
1044+
else logger?.warn?.(message, { name: row.name, error: String((e as Error)?.message ?? e) });
10391045
}
10401046
}
10411047
} else if (recordDiffersFromBody(row, effective)) {

0 commit comments

Comments
 (0)