Skip to content

Commit 0780e88

Browse files
os-billclaude
andauthored
fix(objectql): report a refused write at warn, not error — the caller was already told (#17211)
* 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 * test(objectql): move the three level readers the full suite found (#17052) Running the whole package rather than the message-matched files found one consumer a message grep could not see, plus two rig defects of my own: - `engine-delete-restricted-locale.test.ts` — #7307's pin that the delete door's `developerMessage` reaches the SERVER LOG. It monkey-patches the logger method and reads the meta by ARGUMENT POSITION, so it never mentions `Delete operation failed` and no grep for that message could find it. Now captures both `warn` and `error`, taking meta from the right position for each, so it stays a statement about the log rather than about one method. - `engine-strict-readonly-warning-truthful.test.ts` — `warns` collected every warn-level message, so the door's entry made each strip assertion count 2. The message isolation this suite already performed in `stripLineLevels()` now backs `warns` too, which is where the strip assertions actually read. - `engine-write-failure-log-level.test.ts` — the delete door was driven with an unrecognised `id` option, so the engine refused the call before reaching the driver and the door logged nothing. Addressed through `where`, which is what #4371's refusal message names. Full suite: 289/289 files, 4865/4865 tests. typecheck: OK. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019wtfW1ZxGnP1XKGc9uZVms --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent d03c3c9 commit 0780e88

9 files changed

Lines changed: 475 additions & 59 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-delete-restricted-locale.test.ts

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -220,14 +220,28 @@ describe('#7307 DELETE_RESTRICTED — user copy vs developer guidance', () => {
220220
});
221221

222222
it('reaches the SERVER LOG, so a zh-CN deployment does not log its operator half in Chinese', async () => {
223+
// [#17052] The delete door reports at `warn` now — its catch ends in
224+
// `throw e`, so the caller was told and the entry is not a
225+
// degradation report. What #7307 holds is unchanged and is what is
226+
// asserted below: the operator half reaches the server log, in
227+
// English, in the entry's meta. Only the channel moved, and with it
228+
// the meta's ARGUMENT POSITION — `warn(message, meta)` has no
229+
// `Error` slot in front of it, so meta is the second argument.
230+
// Both levels are captured, so this stays a statement about the log
231+
// rather than about one method: if the entry ever moves again, or
232+
// is emitted twice, this still sees it.
223233
const logged: Array<Record<string, unknown>> = [];
224-
const original = (engine as any).logger.error.bind((engine as any).logger);
225-
(engine as any).logger.error = (msg: string, err: unknown, meta: Record<string, unknown>) => {
226-
logged.push(meta ?? {});
227-
return original(msg, err, meta);
234+
const capture = (level: 'warn' | 'error') => {
235+
const original = (engine as any).logger[level].bind((engine as any).logger);
236+
(engine as any).logger[level] = (...args: unknown[]) => {
237+
logged.push((level === 'warn' ? args[1] : args[2]) as Record<string, unknown> ?? {});
238+
return original(...args);
239+
};
228240
};
241+
capture('warn');
242+
capture('error');
229243
await refuseDelete(engine, 'zh-CN');
230-
expect(logged.some((m) => typeof m.developerMessage === 'string'
244+
expect(logged.some((m) => typeof m?.developerMessage === 'string'
231245
&& (m.developerMessage as string).includes("deleteBehavior:'cascade'"))).toBe(true);
232246
});
233247
});

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: 29 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -125,16 +125,24 @@ 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
*/
141+
const isOperationFailedLine = (msg: string): boolean =>
142+
/^(Update|Insert) operation failed$/.test(msg);
143+
134144
function stripLineLevels(o: Observed): string[] {
135-
return o.lines
136-
.filter((l) => !/^(Update|Insert) operation failed$/.test(l.msg))
137-
.map((l) => l.level);
145+
return o.lines.filter((l) => !isOperationFailedLine(l.msg)).map((l) => l.level);
138146
}
139147

140148
async function observeUpdate(data: unknown, options: Record<string, unknown>): Promise<Observed> {
@@ -148,7 +156,14 @@ async function observeUpdate(data: unknown, options: Record<string, unknown>): P
148156
return {
149157
refusedCode,
150158
driverWrites: writes.length,
151-
warns: logger.lines.filter((l: any) => l.level === 'warn').map((l: any) => l.msg),
159+
// [#17052] The operation-level entry is at `warn` now too, so the message
160+
// isolation this suite has always performed has to hold HERE as well —
161+
// `warns` is the accessor every strip assertion counts, and a second warn
162+
// line that is not the strip's would make every `toHaveLength(1)` a
163+
// statement about the engine's door instead of about the strip.
164+
warns: logger.lines
165+
.filter((l: any) => l.level === 'warn' && !isOperationFailedLine(l.msg))
166+
.map((l: any) => l.msg),
152167
lines: logger.lines.filter((l: any) => l.level !== 'debug' && l.level !== 'info'),
153168
};
154169
}
@@ -164,7 +179,9 @@ async function observeInsert(data: unknown, options: Record<string, unknown> = {
164179
return {
165180
refusedCode,
166181
driverWrites: writes.filter((w) => w.fn === 'create').length,
167-
warns: logger.lines.filter((l: any) => l.level === 'warn').map((l: any) => l.msg),
182+
warns: logger.lines
183+
.filter((l: any) => l.level === 'warn' && !isOperationFailedLine(l.msg))
184+
.map((l: any) => l.msg),
168185
lines: logger.lines.filter((l: any) => l.level !== 'debug' && l.level !== 'info'),
169186
};
170187
}
@@ -201,7 +218,9 @@ describe('#8214 — a strict refusal is not reported as a commit (UPDATE)', () =
201218
expect(stripLineLevels(o)).toEqual(['warn']); // the level its docblock argues
202219
// …and the refusal itself is reported separately, at `error`, by the
203220
// 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);
221+
expect(o.lines.some((l) => l.level === 'warn' && l.msg === 'Update operation failed')).toBe(true);
222+
// …and it is no longer ALSO at `error` — the move is a move, not a copy.
223+
expect(o.lines.some((l) => l.level === 'error')).toBe(false);
205224
expect(o.warns[0]).toContain("Field 'locked_note'"); // the field, named
206225
expect(o.warns[0]).toContain('{ context: { isSystem: true } }'); // a remedy
207226
});
@@ -248,7 +267,8 @@ describe('#8214 — the INSERT side carries the identical defect (the card marke
248267
}),
249268
);
250269
expect(stripLineLevels(o)).toEqual(['warn']);
251-
expect(o.lines.some((l) => l.level === 'error' && l.msg === 'Insert operation failed')).toBe(true);
270+
expect(o.lines.some((l) => l.level === 'warn' && l.msg === 'Insert operation failed')).toBe(true);
271+
expect(o.lines.some((l) => l.level === 'error')).toBe(false);
252272
});
253273

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

0 commit comments

Comments
 (0)