Skip to content

Commit 7d41db6

Browse files
committed
fix(plugin-audit): a multi-row update naming the comment parent pair over rows on different records is refused whole, VALIDATION_ERROR / 400 (ADR-0139 E7)
Claude-Session: https://claude.ai/code/session_01NX4Woa8XhvMs6xpqQpx8kK Co-authored-by: Claude <noreply@anthropic.com>
1 parent 6374550 commit 7d41db6

5 files changed

Lines changed: 99 additions & 6 deletions

File tree

‎.changeset/23857-comment-parent-pair.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ feat(plugin-audit): `sys_comment` rows carry a writer-stamped parent pair, `obje
99

1010
Clause-②: yes
1111

12-
`sys_comment` gains two `readonly` text columns and an index over them. They are derived from `thread_id` (`{object}:{record_id}`, split on the first colon) by one planner, `planCommentParentPair`, and `thread_id` stays the key authors write: nothing changes for a writer. A `sys_comment` before-insert hook stamps every inserted row, whoever writes it, over whatever value the writer sent; an update that names `thread_id` (a re-point) is re-stamped from it; and an update that names either column without a thread has the pair re-derived from the row's stored thread, for every caller — a historical import (`preserveAudit`) included, where the engine's update-side `readonly` strip keeps a value — so no writer's value for either column lands. A thread that names no record stamps `null` / `null`, and such a comment stays hidden from every non-system reader, as before.
12+
`sys_comment` gains two `readonly` text columns and an index over them. They are derived from `thread_id` (`{object}:{record_id}`, split on the first colon) by one planner, `planCommentParentPair`, and `thread_id` stays the key authors write: nothing changes for a writer. A `sys_comment` before-insert hook stamps every inserted row, whoever writes it, over whatever value the writer sent; an update that names `thread_id` (a re-point) is re-stamped from it; and an update that names either column without a thread has the pair re-derived from the row's stored thread, for every caller — a historical import (`preserveAudit`) included, where the engine's update-side `readonly` strip keeps a value — so no writer's value for either column lands. A multi-row update that names either column without a thread is refused whole (`VALIDATION_ERROR` / 400, nothing written) when its rows' threads name different records, since one write cannot store each row's own pair; over rows sharing one thread it stores that thread's pair. A thread that names no record stamps `null` / `null`, and such a comment stays hidden from every non-system reader, as before.
1313

1414
The comment-thread read gate (`installCommentReadVisibility`) is now the parent-record gate the activity stream and the compliance ledger use, over the comment's own pair: one branch `{ object_name: P, record_id: { $in: ReadableIds.of(P) } }` per registered object other than `sys_comment`, ORed and ANDed into the read's `where`, on `find`, `findOne`, `count` and `aggregate`. A broad comment read is served every comment the reader may read, on any table size, with a true `total`. What keeps the bounded 2,000-row fail-closed probe, chosen up front by name, never by catching a refusal:
1515

‎packages/plugins/plugin-audit/src/comment-access-hooks.ts‎

Lines changed: 50 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -342,12 +342,51 @@ function stampCommentParentPair(row: Record<string, unknown>): void {
342342
* writer's value lands, the backfill's included. ⛔ The engine's update-side
343343
* `readonly` strip is not the guard here: under `preserveAudit` (a REST
344344
* historical import puts it on an ordinary caller's write) it keeps a
345-
* readonly value. A predicate update naming the pair re-derives a pair per
346-
* row, and one whose rows hold different threads is refused whole by the
347-
* engine (`MULTI_UPDATE_HOOK_KEY_DIVERGENCE`): one SET clause cannot carry
348-
* two pairs. With no stored row to derive from, the pair is dropped from the
349-
* write. An update naming none of the three columns stamps nothing.
345+
* readonly value. A predicate update naming the pair without a thread
346+
* re-derives a pair per matched row over one shared payload, and one SET
347+
* clause carries a single pair to every row, so when the matched rows'
348+
* threads name different records the write is refused whole, before
349+
* anything is written ({@link refusePairDivergence}, `VALIDATION_ERROR` /
350+
* 400): the engine's own key-set divergence check cannot see it, since
351+
* every row assigns the same two keys. With no stored row to derive from,
352+
* the pair is dropped from the write. An update naming none of the three
353+
* columns stamps nothing.
350354
*/
355+
/** The catalog member for a request whose shape is refused (ADR-0112). */
356+
const PAIR_DIVERGENCE_CODE: StandardErrorCode = 'VALIDATION_ERROR';
357+
358+
/** The pair the first row of a predicate update derived, per write (the engine's shared `dispatch.scope`). */
359+
const pairByWrite = new WeakMap<object, CommentParentPair>();
360+
361+
/**
362+
* Refuse a predicate (`per-row`) update whose matched rows derive different
363+
* parent pairs: one `SET` clause cannot carry two pairs, and carrying one row's
364+
* pair to another row would authorize that comment by a record its thread does
365+
* not name. Refused before anything is written (every before-hook runs ahead
366+
* of the one write). A by-id update, and a predicate update whose rows share
367+
* one thread, pass. `VALIDATION_ERROR` / 400: the request's shape is what is
368+
* refused, and the remedy is in the caller's hands.
369+
*/
370+
function refusePairDivergence(dispatch: unknown, pair: CommentParentPair): void {
371+
const d = dispatch as { mode?: unknown; scope?: unknown } | undefined;
372+
if (d?.mode !== 'per-row' || !d.scope || typeof d.scope !== 'object') return;
373+
const first = pairByWrite.get(d.scope);
374+
if (!first) {
375+
pairByWrite.set(d.scope, pair);
376+
return;
377+
}
378+
if (first.object_name === pair.object_name && first.record_id === pair.record_id) return;
379+
const err: any = new Error(
380+
'Refusing a multi-row update of sys_comment that names object_name / record_id without thread_id: the matched ' +
381+
"comments' threads name different records, and one write cannot store each comment's own pair. The pair is " +
382+
"derived from thread_id — update the comments one by one, or name thread_id.",
383+
);
384+
err.code = PAIR_DIVERGENCE_CODE;
385+
err.status = 400;
386+
err.statusCode = 400;
387+
throw err;
388+
}
389+
351390
function installCommentParentPairStamp(engine: CommentAccessEngine): void {
352391
engine.registerHook(
353392
'beforeInsert',
@@ -382,6 +421,12 @@ function installCommentParentPairStamp(engine: CommentAccessEngine): void {
382421
return;
383422
}
384423
const pair = planCommentParentPair(storedThread);
424+
// A predicate update dispatches this handler once per matched row over
425+
// ONE shared payload, and one SET clause carries the last row's values to
426+
// every row. The engine's key-set divergence check cannot see this (every
427+
// row assigns the same two keys), so the pair is compared here, across
428+
// the call's rows, on the scope object the engine shares between them.
429+
refusePairDivergence(ctx?.dispatch, pair);
385430
payload[COMMENT_PARENT_OBJECT_COLUMN] = pair.object_name;
386431
payload[COMMENT_PARENT_RECORD_COLUMN] = pair.record_id;
387432
},

‎packages/plugins/plugin-audit/src/comment-parent-pair.integration.test.ts‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -364,6 +364,25 @@ describe('[ADR-0139 E7] the comment-thread read gate over the stamped parent pai
364364
expect(await atRest(id)).toMatchObject({ object_name: DOC, record_id: other });
365365
});
366366

367+
it('a SYSTEM multi-row update naming the pair over comments on different records is refused whole; over one thread it stores that thread\'s pair', async () => {
368+
const [d1, d2] = ((await engine.find(DOC, { limit: 2, context: SYS })) as Row[]).map((r) => String(r.id));
369+
const a = String((await engine.insert(COMMENT, commentOn(DOC, d1, 'multi a'), { context: SYS })).id);
370+
const b = String((await engine.insert(COMMENT, commentOn(DOC, d2, 'multi b'), { context: SYS })).id);
371+
const c = String((await engine.insert(COMMENT, commentOn(DOC, d1, 'multi c'), { context: SYS })).id);
372+
const outcome = await engine
373+
.update(COMMENT, { object_name: VAULT, record_id: ids.vault }, { multi: true, where: { id: { $in: [a, b] } }, context: SYS } as any)
374+
.then((value) => ({ value }), (error) => ({ error }));
375+
expect(outcome).toHaveProperty('error');
376+
const error = (outcome as { error: any }).error;
377+
expect({ code: error.code, status: error.status }).toEqual({ code: 'VALIDATION_ERROR', status: 400 });
378+
expect(await atRest(a)).toMatchObject({ object_name: DOC, record_id: d1 });
379+
expect(await atRest(b)).toMatchObject({ object_name: DOC, record_id: d2 });
380+
// Rows sharing one thread derive one pair: the write lands, and stores that pair.
381+
await engine.update(COMMENT, { object_name: VAULT, record_id: ids.vault }, { multi: true, where: { id: { $in: [a, c] } }, context: SYS } as any);
382+
expect(await atRest(a)).toMatchObject({ object_name: DOC, record_id: d1 });
383+
expect(await atRest(c)).toMatchObject({ object_name: DOC, record_id: d1 });
384+
});
385+
367386
it('D5.2: the reaction gate keeps the bounded probe — a literal comment-id list, no leaf', async () => {
368387
const reactionWhere: unknown[] = [];
369388
engine.registerMiddleware(async (op: any, next: () => Promise<void>) => {

‎packages/plugins/plugin-audit/src/comment-thread-leaf.test.ts‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,32 @@ describe('[ADR-0139 E7] the parent pair: one planner, stamped on create and on t
270270
expect(dropped).not.toHaveProperty('object_name');
271271
});
272272

273+
it('a multi-row update naming the pair: refused when the rows derive different pairs, passed when they share one', async () => {
274+
const registered: Array<{ event: string; handler: (ctx: any) => any }> = [];
275+
const engine: any = {
276+
registerHook: (event: string, handler: (ctx: any) => any, options: any) => {
277+
if (options?.object === 'sys_comment' && event === 'beforeUpdate') registered.push({ event, handler });
278+
},
279+
getSchema: () => ({ fields: PAIR_FIELDS }),
280+
};
281+
installCommentAccessHooks(engine, () => null, silentLogger());
282+
const dispatchRows = async (threads: string[]) => {
283+
const scope = {};
284+
const payload: Record<string, unknown> = { object_name: 'vault' };
285+
for (const [index, thread] of threads.entries()) {
286+
for (const h of registered) {
287+
await h.handler({
288+
object: 'sys_comment', input: { data: payload, options: {} }, session: { isSystem: true },
289+
previous: { thread_id: thread }, dispatch: { mode: 'per-row', index, scope },
290+
});
291+
}
292+
}
293+
return payload;
294+
};
295+
await expect(dispatchRows(['crm_case:c1', 'crm_case:c2'])).rejects.toMatchObject({ code: 'VALIDATION_ERROR', status: 400 });
296+
expect(await dispatchRows(['crm_case:c1', 'crm_case:c1'])).toEqual({ object_name: 'crm_case', record_id: 'c1' });
297+
});
298+
273299
it('a sys_comment that declares no pair is never stamped (no unknown column is written)', async () => {
274300
const run = hooks(false);
275301
expect(await run('beforeInsert', { thread_id: 'crm_case:c1' })).toEqual({ thread_id: 'crm_case:c1' });

‎packages/plugins/plugin-audit/src/objects/sys-comment.object.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,9 @@ import { ObjectSchema, Field } from '@objectstack/spec/data';
114114
* names either column without a thread has the pair re-derived from the
115115
* row's stored thread — for every caller, because under `preserveAudit` (a
116116
* historical import) the engine's update-side strip keeps a readonly value.
117+
* A multi-row update naming the pair without a thread is refused whole
118+
* (`VALIDATION_ERROR` / 400) when its rows' threads name different records:
119+
* one write cannot store each row's own pair.
117120
* - **Stored rows.** Rows written before the pair existed carry `null` until
118121
* `os migrate comment-parent-pair --apply` stamps them; until then the read
119122
* gate answers them with the bounded thread probe (fail closed), never a

0 commit comments

Comments
 (0)