Skip to content

Commit db42f25

Browse files
committed
fix(objectql): report a refused write at warn, not error (#17052)
`insert`, `update` and `delete` each end their catch with `throw e` and logged the failure at ERROR one statement earlier. AGENTS.md → Degradation log levels names that shape: "a failure handed to the CALLER is not a degradation at all … Do not bolt a `logger.error` onto such a site." The diagnosis two earlier rulings shaped survives the move. `warn(message, meta?)` has no `Error` slot, and `Error.message`/`stack` are non-enumerable, so handing the error over as meta would have serialised `{}`. The doors instead build the `{ error: { message, stack } }` bag `ObjectLogger.write()` builds from the slot, keeping #8682's redaction and #14095's choice of the driver error over the envelope. The rendered line is byte-identical apart from the level word. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019wtfW1ZxGnP1XKGc9uZVms
1 parent 5d12b16 commit db42f25

8 files changed

Lines changed: 438 additions & 49 deletions
Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
---
2+
"@objectstack/objectql": patch
3+
---
4+
5+
fix(objectql): a refused write reports at `warn`, not `error` — the caller was already told (#17052)
6+
7+
`insert`, `update` and `delete` each end their `catch` with `throw e`, then
8+
logged the failure at ERROR one statement earlier. AGENTS.md → *Degradation log
9+
levels* names that exact shape and forbids it: "a failure handed to the CALLER
10+
is not a degradation at all … Do not bolt a `logger.error` onto such a site."
11+
12+
**This moves published behaviour**, which is why it is a changeset rather than a
13+
`skip-changeset`: the level is what an operator greps, and at least one consumer
14+
reads it structurally. `scripts/publish-smoke.sh` fails a boot on any
15+
error-level line (`SMOKE_ERROR_LOG_PATTERN`), and that is how the defect was
16+
found — `@better-auth/oauth-provider` seeds `sys_oauth_resource` in `insertOnly`
17+
mode and documents its `identifier` UNIQUE constraint AS its race-safety
18+
mechanism, catching the collision and continuing at `debug`. Our line was
19+
emitted before that catch ever ran, so a healthy first boot of every fresh
20+
`create-objectstack` project printed `ERROR Insert operation failed` and red-lit
21+
`publish-smoke / packed-tarballs` for six consecutive runs on a candidate whose
22+
auth and CRUD probes were all green.
23+
24+
**Nothing else about the entry moved.** Same message, same `object` meta, same
25+
redaction (#8682: the bound statement and its values stay cut from `message`
26+
and `stack`), same subject (#14095: the entry carries the driver's own error —
27+
a `DuplicateRecordError`'s `cause` — never the envelope, so the failing column,
28+
MySQL's index name and the driver's frames survive). The `Logger` contract gives
29+
an `Error` slot to `error`/`fatal` only, so the engine now builds the
30+
`{ error: { message, stack } }` bag that slot used to build; handing the Error
31+
to `warn` as meta would have serialised `{}`, because those two fields are
32+
non-enumerable. The rendered line is byte-identical apart from the level word,
33+
and that equivalence is pinned rather than asserted.
34+
35+
If you grep your logs for these three messages, keep the message and drop the
36+
level from the pattern. If you alert on error-level lines from `@objectstack/objectql`,
37+
a refused write no longer raises one — the write's exception still does.

‎packages/objectql/src/driver-fault-redaction.test.ts‎

Lines changed: 30 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -751,7 +751,9 @@ describe('#8682 half B — the write-path loggers', () => {
751751
lines,
752752
trace() {}, fatal() {},
753753
debug() {}, info() {},
754-
warn(msg: string) { lines.push({ level: 'warn', msg: String(msg) }); },
754+
// [#17052] `warn` grew a meta capture: the write doors report there now,
755+
// and the redaction this suite pins travels in `meta.error`.
756+
warn(msg: string, meta?: any) { lines.push({ level: 'warn', msg: String(msg), meta }); },
755757
error(msg: string, err?: any, meta?: any) { lines.push({ level: 'error', msg: String(msg), err, meta }); },
756758
child() { return logger; },
757759
};
@@ -814,28 +816,37 @@ describe('#8682 half B — the write-path loggers', () => {
814816
} as any);
815817
} catch (e) { thrown = e; }
816818
const line = logger.lines.find((l: any) => l.msg === 'Insert operation failed');
817-
return { line, thrown };
819+
return { line, thrown, logger };
818820
}
819821

820-
it('the entry survives — same level, same message, same object', async () => {
821-
const { line } = await insertAgainstADriftedColumn();
822+
it('the entry survives — one line, `warn` since #17052, same message, same object', async () => {
823+
const { line, logger } = await insertAgainstADriftedColumn();
822824

823825
expect(line).toBeDefined();
824-
expect(line!.level).toBe('error');
825-
expect(line!.meta).toEqual({ object: 'crm_account' });
826+
// [#17052] Was `error` until the door was read against AGENTS.md's third
827+
// legal answer: this catch rethrows, so the caller was told. What this
828+
// suite pins is the REDACTION, and it is unchanged by the level — the
829+
// assertions below read the same two fields they always did.
830+
expect(line!.level).toBe('warn');
831+
expect(line!.meta).toEqual({
832+
object: 'crm_account',
833+
error: { message: expect.any(String), stack: expect.any(String) },
834+
});
835+
// …and the entry did not merely move: nothing is emitted at `error` now.
836+
expect(logger.lines.filter((l: any) => l.level === 'error')).toHaveLength(0);
826837
});
827838

828839
it('the failing column is still named — the fault stays debuggable', async () => {
829840
const { line } = await insertAgainstADriftedColumn();
830841

831-
expect(String(line!.err?.message)).toContain('has no column named secret_note');
832-
expect(String(line!.err?.stack)).toContain('at Database.prepare');
842+
expect(String(line!.meta?.error?.message)).toContain('has no column named secret_note');
843+
expect(String(line!.meta?.error?.stack)).toContain('at Database.prepare');
833844
});
834845

835846
it('neither `message` nor `stack` carries a caller value', async () => {
836847
const { line } = await insertAgainstADriftedColumn();
837848

838-
for (const field of [String(line!.err?.message), String(line!.err?.stack)]) {
849+
for (const field of [String(line!.meta?.error?.message), String(line!.meta?.error?.stack)]) {
839850
expect(field).not.toContain(SECRET);
840851
expect(field).not.toContain(DESCRIPTION);
841852
expect(field).not.toContain('insert into');
@@ -872,17 +883,22 @@ describe('#8682 half B — the write-path loggers', () => {
872883
const { line } = await insertAgainstADriftedColumn(MYSQL_DUPLICATE_ENTRY);
873884

874885
expect(line).toBeDefined();
875-
expect(line!.level).toBe('error');
876-
expect(line!.meta).toEqual({ object: 'crm_account' });
886+
// [#17052] The level moved to `warn`; #14095's property is what this case
887+
// exists for, and it is asserted below on the same two fields. They travel
888+
// in `meta.error` now because `warn` has no `Error` slot — the engine
889+
// rebuilds the very `{ message, stack }` bag the slot used to build, so
890+
// the index name still arrives.
891+
expect(line!.level).toBe('warn');
892+
expect(line!.meta?.object).toBe('crm_account');
877893
// The operator's answer to "which constraint?" is kept whole.
878-
expect(String(line!.err?.message)).toContain("for key 'crm_account.secret_note'");
879-
expect(String(line!.err?.stack)).toContain('at Database.prepare');
894+
expect(String(line!.meta?.error?.message)).toContain("for key 'crm_account.secret_note'");
895+
expect(String(line!.meta?.error?.stack)).toContain('at Database.prepare');
880896
});
881897

882898
it('MySQL duplicate entry — neither `message` nor `stack` carries the value', async () => {
883899
const { line } = await insertAgainstADriftedColumn(MYSQL_DUPLICATE_ENTRY);
884900

885-
for (const field of [String(line!.err?.message), String(line!.err?.stack)]) {
901+
for (const field of [String(line!.meta?.error?.message), String(line!.meta?.error?.stack)]) {
886902
expect(field).not.toContain(SECRET);
887903
expect(field).not.toContain(DESCRIPTION);
888904
expect(field).not.toContain('insert into');

‎packages/objectql/src/driver-fault-redaction.ts‎

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
* to disk at ERROR level, twice:
1515
*
1616
* ```
17-
* ERROR Insert operation failed {"object":"crm_account","error":{"message":
17+
* WARN Insert operation failed {"object":"crm_account","error":{"message":
1818
* "insert into `crm_account` (`account_number`, …, `zzz_nonexistent_field`)
1919
* values ('ACC-000011', …, 'SENSITIVE-CANARY-9f3a2b') returning *
2020
* - table crm_account has no column named zzz_nonexistent_field", "stack":
@@ -32,10 +32,17 @@
3232
* column and the condition (`table crm_account has no column named
3333
* zzz_nonexistent_field`, `NOT NULL constraint failed: sys_team.name`). It is
3434
* kept, and the log site keeps the `object` it already carried. ⛔ The
35-
* remedy for this exposure is NOT to lower the level or drop the entry: a
36-
* driver-level fault that logs nothing is a fault nobody can debug, which is
37-
* strictly worse than one logged too loudly. This narrows WHAT is written; the
38-
* level, the message and the entry are untouched.
35+
* remedy for this exposure is NOT to drop the entry: a driver-level fault that
36+
* logs nothing is a fault nobody can debug, which is strictly worse than one
37+
* logged too loudly. This narrows WHAT is written; the message and the entry
38+
* are untouched.
39+
*
40+
* [#17052] The LEVEL has since moved to `warn` on the three write doors — not
41+
* as a remedy for THIS exposure, which it would not have fixed, but because
42+
* those catches rethrow and AGENTS.md rules a failure handed to the caller is
43+
* not a degradation report. The specimen above therefore reads `WARN` today.
44+
* Nothing else about it moved: the same redacted `message`/`stack` reach the
45+
* same `error` key of the same meta bag (`ObjectQL.writeFailureLogMeta`).
3946
*
4047
* ## [#8823] …but "the tail names identifiers" is not true of every dialect
4148
*

‎packages/objectql/src/engine-find-missing-table-log-level.test.ts‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -323,18 +323,26 @@ describe('engine `find` failure log level is chosen by CAUSE (#13273)', () => {
323323
);
324324
});
325325

326-
it('a WRITE to a table that does not exist is still `error`', async () => {
327-
// Nothing landed, and the row the caller believes it stored is gone —
328-
// never a normal answer, whatever the cause.
326+
it('a WRITE to a table that does not exist reports ONCE, and not at `debug`', async () => {
327+
// [#17052] This case is unchanged in the two things it was written to
328+
// hold — the write door reports, and it does not join the read door's
329+
// `debug` demotion — and changed in the one it also happened to state:
330+
// the level is `warn`, because the write catch rethrows and the caller
331+
// was told. The demotion this suite governs is still READS ONLY: a
332+
// missing table earns no quieter treatment on a write than any other
333+
// driver fault does, and `warn` is what every write fault takes now.
329334
await boot(() => envelope(new Error(`no such table: ${OBJECT}`)));
330335

331336
await expect(engine.insert(OBJECT, { label: 'x' } as any)).rejects.toThrow();
332337

333-
const insertFrames = logger.lines.error.filter(
338+
const insertFrames = logger.lines.warn.filter(
334339
(l: any) => l.msg === 'Insert operation failed',
335340
);
336341
expect(insertFrames).toHaveLength(1);
337342
expect(logger.lines.debug.filter((l: any) => l.msg === 'Insert operation failed')).toHaveLength(0);
343+
// ⭐ The read door's classification did NOT leak onto the write door:
344+
// there is no second, quieter entry and no `error` entry either.
345+
expect(logger.lines.error.filter((l: any) => l.msg === 'Insert operation failed')).toHaveLength(0);
338346
});
339347
});
340348
});

‎packages/objectql/src/engine-strict-readonly-warning-truthful.test.ts‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -125,11 +125,18 @@ interface Observed {
125125

126126
/**
127127
* The level of the STRIP's own line, isolated from the engine's pre-existing
128-
* `'Update operation failed'` / `'Insert operation failed'` ERROR — which a
128+
* `'Update operation failed'` / `'Insert operation failed'` entry — which a
129129
* strict refusal legitimately emits, because the caller really was handed a
130130
* failure. The two lines are different facts and this suite must not conflate
131131
* them: the strip's line stays at `warn` (its docblock argues that level), and
132-
* the operation-level error is nobody's business here.
132+
* the operation-level entry is nobody's business here.
133+
*
134+
* [#17052] That operation-level entry moved from `error` to `warn`, which makes
135+
* the isolation this helper performs load-bearing rather than merely tidy: the
136+
* two lines now share a level, so only the MESSAGE tells them apart. The filter
137+
* was already keyed on the message, so it needed no change — but the two
138+
* assertions below that named `error` did, and they now assert the level the
139+
* door actually takes rather than the one it used to.
133140
*/
134141
function stripLineLevels(o: Observed): string[] {
135142
return o.lines
@@ -201,7 +208,9 @@ describe('#8214 — a strict refusal is not reported as a commit (UPDATE)', () =
201208
expect(stripLineLevels(o)).toEqual(['warn']); // the level its docblock argues
202209
// …and the refusal itself is reported separately, at `error`, by the
203210
// engine — two lines, two facts, neither pretending to be the other.
204-
expect(o.lines.some((l) => l.level === 'error' && l.msg === 'Update operation failed')).toBe(true);
211+
expect(o.lines.some((l) => l.level === 'warn' && l.msg === 'Update operation failed')).toBe(true);
212+
// …and it is no longer ALSO at `error` — the move is a move, not a copy.
213+
expect(o.lines.some((l) => l.level === 'error')).toBe(false);
205214
expect(o.warns[0]).toContain("Field 'locked_note'"); // the field, named
206215
expect(o.warns[0]).toContain('{ context: { isSystem: true } }'); // a remedy
207216
});
@@ -248,7 +257,8 @@ describe('#8214 — the INSERT side carries the identical defect (the card marke
248257
}),
249258
);
250259
expect(stripLineLevels(o)).toEqual(['warn']);
251-
expect(o.lines.some((l) => l.level === 'error' && l.msg === 'Insert operation failed')).toBe(true);
260+
expect(o.lines.some((l) => l.level === 'warn' && l.msg === 'Insert operation failed')).toBe(true);
261+
expect(o.lines.some((l) => l.level === 'error')).toBe(false);
252262
});
253263

254264
it('the DEFAULT insert strip still says COMMITTED WITHOUT IT — and the row really does commit', async () => {

‎packages/objectql/src/engine-update-duplicate-record.test.ts‎

Lines changed: 40 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -194,17 +194,32 @@ function makeDriver(opts: DriverOpts = {}) {
194194
return driver as IDataDriver & { update: any; updateMany: any };
195195
}
196196

197-
/** A logger that records what the door's `Update operation failed` line carried. */
197+
/**
198+
* A logger that records what the door's `Update operation failed` line carried.
199+
*
200+
* [#17052] The line is emitted at `warn` now (the catch rethrows — the caller
201+
* was told), so the recorder listens there. `warn` has no `Error` slot, so the
202+
* driver's error arrives as `meta.error = { message, stack }`; `err` below is
203+
* lifted back out of it, which keeps every assertion in section (7) reading the
204+
* same two fields #14095 pinned. `errors` also captures anything still emitted
205+
* at `error`, so a regression that puts the line back would be visible rather
206+
* than silently unmatched.
207+
*/
198208
function recordingLogger() {
199-
const errors: Array<{ msg: string; err: unknown; meta: unknown }> = [];
209+
const errors: Array<{ msg: string; err: unknown; meta: unknown; level: string }> = [];
210+
const record = (level: string) => (msg: string, a?: unknown, b?: unknown) => {
211+
const meta = (level === 'warn' ? a : b) as Record<string, any> | undefined;
212+
const err = level === 'warn'
213+
? (meta?.error as unknown)
214+
: a;
215+
errors.push({ msg, err, meta, level });
216+
};
200217
const logger = {
201218
info: vi.fn(),
202219
debug: vi.fn(),
203-
warn: vi.fn(),
220+
warn: vi.fn(record('warn')),
204221
trace: vi.fn(),
205-
error: vi.fn((msg: string, err?: unknown, meta?: unknown) => {
206-
errors.push({ msg, err, meta });
207-
}),
222+
error: vi.fn(record('error')),
208223
};
209224
return { logger, errors };
210225
}
@@ -603,14 +618,19 @@ describe('engine.update — a driver unique violation is a DUPLICATE_RECORD enve
603618

604619
const line = errors.find((e) => e.msg === 'Update operation failed');
605620
expect(line).toBeDefined();
606-
const logged = line!.err as Error;
607-
expect(logged).toBeInstanceOf(Error);
621+
// [#17052] …at `warn`: the catch rethrows, so the caller was told and
622+
// this entry is the operator's copy, not a degradation report.
623+
expect(line!.level).toBe('warn');
624+
const logged = line!.err as { message: string; stack: string };
608625
expect(logged.message).toContain('UNIQUE constraint failed: doc.email');
609626
expect(logged.message).not.toContain('Duplicate record refused');
610627
// …and the redaction still holds: no statement, no bound value.
611628
expect(logged.message).not.toMatch(/update `doc` set/i);
612629
expect(logged.message).not.toContain('a@b.example');
613-
expect(line!.meta).toEqual({ object: 'doc' });
630+
// The `cause` reached the log with its FRAMES, which is the other half of
631+
// what the platform logger serializes.
632+
expect(typeof logged.stack).toBe('string');
633+
expect(line!.meta).toEqual({ object: 'doc', error: logged });
614634
});
615635

616636
it('logs the same way on the predicate door', async () => {
@@ -622,8 +642,9 @@ describe('engine.update — a driver unique violation is a DUPLICATE_RECORD enve
622642

623643
const line = errors.find((e) => e.msg === 'Update operation failed');
624644
expect(line).toBeDefined();
625-
expect((line!.err as Error).message).toContain('violates unique constraint');
626-
expect((line!.err as Error).message).not.toContain('Duplicate record refused');
645+
expect(line!.level).toBe('warn');
646+
expect((line!.err as { message: string }).message).toContain('violates unique constraint');
647+
expect((line!.err as { message: string }).message).not.toContain('Duplicate record refused');
627648
});
628649

629650
it('a non-enveloped failure is logged exactly as before — the driver error itself', async () => {
@@ -635,7 +656,14 @@ describe('engine.update — a driver unique violation is a DUPLICATE_RECORD enve
635656

636657
const line = errors.find((e) => e.msg === 'Update operation failed');
637658
expect(line).toBeDefined();
638-
expect(line!.err).toBe(raw);
659+
// [#17052] The identity check this used to make (`err === raw`) was a
660+
// check on the SLOT, which `warn` does not have. What it was there to
661+
// prove — nothing rewrites a failure that is not a duplicate envelope —
662+
// is asserted on the two fields the platform logger has ever serialized,
663+
// byte for byte against the driver's own error.
664+
expect(line!.err).toEqual({ message: raw.message, stack: raw.stack });
665+
// …and the caller still receives that very object, unwrapped.
666+
expect(line!.level).toBe('warn');
639667
});
640668
});
641669
});

0 commit comments

Comments
 (0)