Skip to content

Commit c5e7bd9

Browse files
os-zhuangclaude
andauthored
fix(plugin-audit): report where the audit system tables were provisioned (#4887) (#5035)
`provisionSystemTables()` said nothing on success and returned silently when the engine exposed no `syncObjectSchema`, so "provisioned three tables" and "provisioned nothing" produced byte-identical logs. `syncObjectSchema()` itself returns `void` with three silent exits of its own (object unregistered / no driver / driver without `syncSchema`), none of which throw, so the per-object `catch` could not observe them either. #4887 is what that silence costs. `sys_audit_log` (`lifecycle.class: 'audit'`) and `sys_activity` (`lifecycle.class: 'telemetry'`) were reported as never provisioned because they were absent from the primary SQLite file. They were provisioned: ADR-0057 §3.6 routes both to the dedicated `telemetry` datasource whenever one is registered, and `os dev` registers one by default as a sibling file (`dev.db` -> `dev.telemetry.db`). `sys_comment` carries no lifecycle class, stays on the primary, and was the one that "existed". Nothing in the log connected those facts. Provisioning now reports itself: - the wholesale skip is a `warn` naming the consequence (tables stay lazy-created on first WRITE; a read-first env logs "no such table"); - one `info` line per boot listing where each table landed, resolved through the engine's own `getDriverForObject`; - a second `info` line when the ADR-0057 split is in effect, stating that those tables live in another store and that anything reading them without naming the object will report "no such table" even though provisioning succeeded; - an object that resolves to no driver is a `warn` — `syncObjectSchema()` issues no DDL in that case and throws nothing, so from outside the engine this is the only place it can be observed. Behaviour is otherwise unchanged: the same three objects are synced, per-object failures stay isolated, and an engine without on-demand DDL still degrades rather than failing `start()`. Refs #4887 Claude-Session: https://claude.ai/code/session_015W6nhsDrz6zWQc8je12a1t Co-authored-by: Claude <noreply@anthropic.com>
1 parent 70c0769 commit c5e7bd9

3 files changed

Lines changed: 276 additions & 3 deletions

File tree

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
---
2+
"@objectstack/plugin-audit": patch
3+
---
4+
5+
fix(plugin-audit): say where the audit system tables were provisioned, and stop skipping provisioning silently (#4887)
6+
7+
`AuditPlugin.provisionSystemTables()` created `sys_audit_log` / `sys_activity` /
8+
`sys_comment` at `kernel:ready` and then said **nothing** — not on success, and
9+
not when it skipped the work entirely (`typeof engine.syncObjectSchema !==
10+
'function'` returned silently). `syncObjectSchema()` itself returns `void` and
11+
has three silent exits of its own — the object is not in the registry, no driver
12+
resolves for it, or the resolved driver has no `syncSchema` — none of which
13+
throw. So "provisioned three tables" and "provisioned nothing at all" produced
14+
byte-identical logs, and the only way to tell them apart was to go looking in a
15+
database.
16+
17+
#4887 is what that costs. `sys_audit_log` and `sys_activity` were reported as
18+
never provisioned because they were absent from the primary SQLite file, with
19+
the silent `typeof` bail named as the likely cause. Neither was true:
20+
`sys_audit_log` (`lifecycle.class: 'audit'`) and `sys_activity`
21+
(`lifecycle.class: 'telemetry'`) are routed by **ADR-0057 §3.6** to the
22+
dedicated `telemetry` datasource whenever one is registered, and `os dev`
23+
registers one by default as a *sibling file* (`dev.db``dev.telemetry.db`).
24+
Both tables had been created — in the other store. `sys_comment` carries no
25+
lifecycle class, stays on the primary, and was the one that "existed". Nothing
26+
in the log connected those three facts.
27+
28+
Provisioning now reports itself:
29+
30+
- **Wholesale skip is a `warn`, naming the consequence** — the tables stay
31+
lazy-created on first WRITE, so an env that READS one first (the home page
32+
activity feed queries `sys_activity` before any mutation) logs "no such
33+
table" until something writes.
34+
- **One `info` line per boot listing where each table landed**
35+
`sys_audit_log→telemetry, sys_activity→telemetry, sys_comment→sqlite`,
36+
resolved through the engine's own `getDriverForObject`, so the log states the
37+
routing rather than leaving it to be inferred.
38+
- **A second `info` line when the ADR-0057 split is in effect**, saying
39+
explicitly that those tables live in a different store — on SQLite, a
40+
different *file* — and that anything reading them without naming the object
41+
(raw SQL against the default datasource) will report "no such table" even
42+
though provisioning succeeded.
43+
- **An object that resolves to no driver is a `warn`**`syncObjectSchema()`
44+
returns without issuing any DDL in that case and throws nothing, so the
45+
per-object `catch` never fires; from outside the engine this is the only place
46+
it can be observed.
47+
48+
Behaviour is otherwise unchanged: the same three objects are synced, per-object
49+
failures stay isolated, and an engine without on-demand DDL still degrades
50+
instead of failing `start()`.

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

Lines changed: 148 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,14 @@ function makeCtx(engine: unknown) {
3939
['manifest', { register() {} }],
4040
]);
4141
const readyHooks: Array<() => Promise<void> | void> = [];
42+
// #4887 — the log IS the deliverable for the provisioning path: its silence
43+
// is what made a working-but-elsewhere table read as a never-created one.
44+
// Capture info/warn so the tests can assert on what an operator would see.
45+
const logs = { info: [] as string[], warn: [] as string[] };
4246
const logger = {
43-
info() {}, warn() {}, error() {}, debug() {},
47+
info(msg: string) { logs.info.push(String(msg)); },
48+
warn(msg: string) { logs.warn.push(String(msg)); },
49+
error() {}, debug() {},
4450
child() { return logger; },
4551
};
4652
const ctx = {
@@ -51,7 +57,7 @@ function makeCtx(engine: unknown) {
5157
if (event === 'kernel:ready') readyHooks.push(fn);
5258
},
5359
} as any;
54-
return { ctx, fireReady: async () => { for (const fn of readyHooks) await fn(); } };
60+
return { ctx, logs, fireReady: async () => { for (const fn of readyHooks) await fn(); } };
5561
}
5662

5763
describe('AuditPlugin — system table provisioning', () => {
@@ -89,6 +95,146 @@ describe('AuditPlugin — system table provisioning', () => {
8995
});
9096
});
9197

98+
/**
99+
* #4887 — provisioning must SAY what it did.
100+
*
101+
* `syncObjectSchema` returns `void` and has three silent exits of its own
102+
* (object not registered / no driver / driver without `syncSchema`), so a
103+
* caller that only catches throws cannot distinguish "created the table" from
104+
* "did nothing". Combined with a silent `typeof sync !== 'function'` bail on
105+
* this side, a boot where provisioning was skipped WHOLESALE logged exactly
106+
* the same thing as a boot where it worked: nothing.
107+
*
108+
* #4887 is what that costs. `sys_audit_log` / `sys_activity` were reported as
109+
* "never provisioned" because they were absent from the primary SQLite file —
110+
* but ADR-0057 §3.6 routes both (lifecycle classes `audit` / `telemetry`) to
111+
* the `telemetry` datasource when one is registered, and `os dev` registers one
112+
* by default as a sibling file. The tables existed; the log just never said
113+
* where. These tests pin the three statements an operator now gets.
114+
*/
115+
describe('AuditPlugin — provisioning is audible (#4887)', () => {
116+
/** Engine whose datasource routing mirrors ADR-0057 §3.6 in `os dev`. */
117+
function makeRoutingEngine(routes: Record<string, string | undefined>, defaultName = 'sqlite') {
118+
return {
119+
async syncObjectSchema(_name: string) { /* DDL issued on the resolved driver */ },
120+
getDriverForObject(name: string) {
121+
const ds = routes[name];
122+
return ds === undefined ? undefined : { name: ds };
123+
},
124+
getDefaultDriverName() { return defaultName; },
125+
};
126+
}
127+
128+
it('warns — instead of returning silently — when the engine has no syncObjectSchema', async () => {
129+
const { ctx, logs, fireReady } = makeCtx({ async find() { return []; } });
130+
const plugin = new AuditPlugin();
131+
await plugin.init(ctx);
132+
await plugin.start(ctx);
133+
await fireReady();
134+
135+
const warned = logs.warn.find((m) => m.includes('no syncObjectSchema'));
136+
expect(warned).toBeDefined();
137+
// The warning must name the CONSEQUENCE, not just the missing method:
138+
// nothing is provisioned and a read-first env logs "no such table".
139+
expect(warned).toMatch(/sys_activity/);
140+
expect(warned).toMatch(/no such table/);
141+
});
142+
143+
it('reports the datasource each system table was provisioned into', async () => {
144+
// The exact `os dev` shape: audit + activity split off to `telemetry`,
145+
// comment stays on the primary.
146+
const engine = makeRoutingEngine({
147+
sys_audit_log: 'telemetry',
148+
sys_activity: 'telemetry',
149+
sys_comment: 'sqlite',
150+
});
151+
const { ctx, logs, fireReady } = makeCtx(engine);
152+
const plugin = new AuditPlugin();
153+
await plugin.init(ctx);
154+
await plugin.start(ctx);
155+
await fireReady();
156+
157+
const placement = logs.info.find((m) => m.includes('system tables provisioned'));
158+
expect(placement).toBeDefined();
159+
expect(placement).toContain('sys_audit_log→telemetry');
160+
expect(placement).toContain('sys_activity→telemetry');
161+
expect(placement).toContain('sys_comment→sqlite');
162+
163+
// …and the split itself is called out, because "absent from the database I
164+
// am looking at" is not "never created".
165+
const split = logs.info.find((m) => m.includes('NON-default datasource'));
166+
expect(split).toBeDefined();
167+
expect(split).toContain('ADR-0057');
168+
expect(split).toContain('sys_audit_log→telemetry');
169+
expect(split).toContain('sys_activity→telemetry');
170+
// sys_comment is ON the default datasource — it must not be listed as split.
171+
expect(split).not.toContain('sys_comment');
172+
});
173+
174+
it('says nothing about a split when every table is on the default datasource', async () => {
175+
const engine = makeRoutingEngine({
176+
sys_audit_log: 'sqlite',
177+
sys_activity: 'sqlite',
178+
sys_comment: 'sqlite',
179+
});
180+
const { ctx, logs, fireReady } = makeCtx(engine);
181+
const plugin = new AuditPlugin();
182+
await plugin.init(ctx);
183+
await plugin.start(ctx);
184+
await fireReady();
185+
186+
expect(logs.info.find((m) => m.includes('system tables provisioned'))).toBeDefined();
187+
expect(logs.info.some((m) => m.includes('NON-default datasource'))).toBe(false);
188+
expect(logs.warn.some((m) => m.includes('NO datasource driver'))).toBe(false);
189+
});
190+
191+
it("warns when an object resolves to no driver — syncObjectSchema's own silent exit", async () => {
192+
// `syncObjectSchema` returns without issuing DDL when no driver backs the
193+
// object. It throws nothing, so the per-object catch never fires: the only
194+
// way this is ever visible is from the outside, here.
195+
const engine = makeRoutingEngine({
196+
sys_audit_log: undefined,
197+
sys_activity: 'sqlite',
198+
sys_comment: 'sqlite',
199+
});
200+
const { ctx, logs, fireReady } = makeCtx(engine);
201+
const plugin = new AuditPlugin();
202+
await plugin.init(ctx);
203+
await plugin.start(ctx);
204+
await fireReady();
205+
206+
const warned = logs.warn.find((m) => m.includes('NO datasource driver'));
207+
expect(warned).toBeDefined();
208+
expect(warned).toContain('sys_audit_log');
209+
// The other two still provisioned — one unroutable object does not stop them.
210+
const placement = logs.info.find((m) => m.includes('system tables provisioned'));
211+
expect(placement).toContain('sys_activity→sqlite');
212+
expect(placement).toContain('sys_comment→sqlite');
213+
expect(placement).not.toContain('sys_audit_log');
214+
});
215+
216+
it('keeps reporting placements when one object fails to sync', async () => {
217+
const engine = {
218+
async syncObjectSchema(name: string) {
219+
if (name === 'sys_activity') throw new Error('disk I/O error');
220+
},
221+
getDriverForObject() { return { name: 'sqlite' }; },
222+
getDefaultDriverName() { return 'sqlite'; },
223+
};
224+
const { ctx, logs, fireReady } = makeCtx(engine);
225+
const plugin = new AuditPlugin();
226+
await plugin.init(ctx);
227+
await plugin.start(ctx);
228+
await fireReady();
229+
230+
expect(logs.warn.some((m) => m.includes('could not provision sys_activity'))).toBe(true);
231+
const placement = logs.info.find((m) => m.includes('system tables provisioned'));
232+
expect(placement).toContain('sys_audit_log→sqlite');
233+
expect(placement).toContain('sys_comment→sqlite');
234+
expect(placement).not.toContain('sys_activity');
235+
});
236+
});
237+
92238
/**
93239
* #4630 — the sys_comment record-level gates are only worth as much as their
94240
* MOUNTING: `comment-access-hooks.test.ts` proves what the hooks decide, this

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

Lines changed: 78 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -181,19 +181,96 @@ export class AuditPlugin implements Plugin {
181181
* it is absent (and alters to add columns) — so this is safe on every boot,
182182
* and a no-op for objects whose table already exists. Per-object failures are
183183
* isolated so one bad object can't block the rest.
184+
*
185+
* ## Why this method reports where each table landed (#4887)
186+
*
187+
* `syncObjectSchema` returns `void` and exits SILENTLY on three conditions
188+
* the plugin cannot see from the outside: the object is not in the registry,
189+
* no driver resolves for it, or the resolved driver has no `syncSchema`. A
190+
* caller that only catches throws therefore cannot tell "created" from "did
191+
* nothing" — and neither could a reader of the log, because this method said
192+
* nothing at all on success.
193+
*
194+
* That silence cost a whole misdiagnosis. #4887 reported these tables as
195+
* "never provisioned" because they were absent from the primary SQLite file,
196+
* and concluded the guard below had bailed out. It had not: `sys_audit_log`
197+
* (`lifecycle.class: 'audit'`) and `sys_activity` (`lifecycle.class:
198+
* 'telemetry'`) are routed by ADR-0057 §3.6 to the dedicated `telemetry`
199+
* datasource whenever one is registered — which `os dev` provisions by
200+
* default as a SIBLING FILE (`dev.db` → `dev.telemetry.db`). Their tables
201+
* were created, in that other store. `sys_comment` carries no lifecycle
202+
* class, stays on the primary, and was the one the reporter found. So the
203+
* provisioning loop reports the resolved datasource per object, and calls out
204+
* the split explicitly when it is in effect: a table that is "missing" from
205+
* the database you are looking at, and a table that was never created, are
206+
* different problems, and the log now distinguishes them.
184207
*/
185208
private async provisionSystemTables(engine: IDataEngine, ctx: PluginContext): Promise<void> {
186209
// `syncObjectSchema` lives on the concrete ObjectQL engine, not the
187210
// IDataEngine contract; engines/drivers without on-demand DDL (e.g. an
188211
// in-memory test double) simply skip provisioning.
189212
const sync = (engine as unknown as { syncObjectSchema?: (name: string) => Promise<void> }).syncObjectSchema;
190-
if (typeof sync !== 'function') return;
213+
if (typeof sync !== 'function') {
214+
// #4887 — this return used to be silent, so "provisioning was skipped
215+
// wholesale" and "provisioning ran fine" produced identical logs. Name
216+
// the consequence, not just the condition.
217+
ctx.logger.warn(
218+
'AuditPlugin: this engine exposes no syncObjectSchema() — sys_audit_log / sys_activity / sys_comment were NOT ' +
219+
'provisioned up-front and stay lazy-created on first WRITE. An env that READS one first (the home page ' +
220+
'activity feed queries sys_activity before any mutation) will log "no such table" until something writes to it.',
221+
);
222+
return;
223+
}
224+
// Same optional-probe posture as `syncObjectSchema` above: `getDriverForObject`
225+
// is public on the concrete ObjectQL engine but not part of IDataEngine, so
226+
// engines that lack it simply report no datasource — never an error.
227+
const resolveDriver = (engine as unknown as {
228+
getDriverForObject?: (name: string) => { name?: string } | undefined;
229+
}).getDriverForObject;
230+
// Declared on IDataEngine (optional — engines with no named-driver registry
231+
// omit it), so no cast is needed here.
232+
const defaultDatasource = engine.getDefaultDriverName?.();
233+
234+
const placements: string[] = [];
235+
const offDefault: string[] = [];
191236
for (const obj of [SysAuditLog, SysActivity, SysComment]) {
192237
try {
193238
await sync.call(engine, obj.name);
194239
} catch (err) {
195240
ctx.logger.warn(`AuditPlugin: could not provision ${obj.name} storage — ${(err as Error)?.message ?? err}`);
241+
continue;
196242
}
243+
if (typeof resolveDriver !== 'function') continue;
244+
let datasource: string | undefined;
245+
try {
246+
datasource = resolveDriver.call(engine, obj.name)?.name;
247+
} catch {
248+
datasource = undefined;
249+
}
250+
if (!datasource) {
251+
// The second of the two silent exits #4887 asked to make audible: the
252+
// call above resolved without throwing, but no driver backs this object,
253+
// so `syncObjectSchema` returned having issued no DDL at all.
254+
ctx.logger.warn(
255+
`AuditPlugin: ${obj.name} resolves to NO datasource driver — syncObjectSchema() returned without creating its ` +
256+
'storage. Reads and writes against it will fail with "no such table" until a driver backs its datasource.',
257+
);
258+
continue;
259+
}
260+
placements.push(`${obj.name}${datasource}`);
261+
if (defaultDatasource !== undefined && datasource !== defaultDatasource) offDefault.push(`${obj.name}${datasource}`);
262+
}
263+
264+
if (placements.length > 0) {
265+
ctx.logger.info(`AuditPlugin: system tables provisioned — ${placements.join(', ')}`);
266+
}
267+
if (offDefault.length > 0) {
268+
ctx.logger.info(
269+
`AuditPlugin: ${offDefault.join(', ')} live on a NON-default datasource (ADR-0057 §3.6 lifecycle-class ` +
270+
`separation), not on '${defaultDatasource}'. Their tables exist in that store — on SQLite, a different FILE. ` +
271+
'Anything that reads them without naming the object (raw SQL on the default datasource) will report ' +
272+
'"no such table" even though provisioning succeeded.',
273+
);
197274
}
198275
}
199276
}

0 commit comments

Comments
 (0)