Skip to content

Commit 32b6f53

Browse files
committed
fix(plugin-audit): an update naming the comment parent pair without a thread re-derives it from the stored thread, so preserveAudit cannot land a writer's pair (ADR-0139 E7)
Claude-Session: https://claude.ai/code/session_01NX4Woa8XhvMs6xpqQpx8kK Co-authored-by: Claude <noreply@anthropic.com>
1 parent e499ada commit 32b6f53

8 files changed

Lines changed: 94 additions & 30 deletions

File tree

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

Lines changed: 2 additions & 2 deletions
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; a non-system caller's own value for either column is dropped by the engine on update. 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 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

@@ -22,4 +22,4 @@ The comment-thread read gate (`installCommentReadVisibility`) is now the parent-
2222

2323
Comment reactions are unchanged: their only parent is `sys_comment`, which this gate narrows by an object-scoped middleware, so the reaction gate keeps the bounded probe (ADR-0139 D5.2).
2424

25-
`installCommentReadVisibility` takes an optional third argument, the security source whose `canReadObject` admits a leaf; without it every comment read is the bounded probe, as before. `CommentAccessEngine` gains an optional `getSchema`. `@objectstack/plugin-audit` exports the backfill, `backfillCommentParentPair` (with `COMMENT_PARENT_PAIR_BACKFILL_PAGE_SIZE` and its report and engine types), the planner `planCommentParentPair` (with `COMMENT_PARENT_OBJECT_COLUMN`, `COMMENT_PARENT_RECORD_COLUMN` and the `CommentParentPair` type).
25+
`installCommentReadVisibility` takes an optional third argument, the security source whose `canReadObject` admits a leaf (`ParentObjectReadAdmissionSource`); without it every comment read is the bounded probe, as before. Its first parameter widens from `CommentAccessEngine` to `ParentRecordGateEngine` (`CommentAccessEngine` plus three optional members: a registry view, `hasObjectMiddleware` and `resolveEffectiveDatasource`); every existing caller's engine still satisfies it. Both types reach the published declaration through that signature. `CommentAccessEngine` gains an optional `getSchema`. `@objectstack/plugin-audit` exports the backfill, `backfillCommentParentPair` (with `COMMENT_PARENT_PAIR_BACKFILL_PAGE_SIZE` and its report and engine types), the planner `planCommentParentPair` (with `COMMENT_PARENT_OBJECT_COLUMN`, `COMMENT_PARENT_RECORD_COLUMN` and the `CommentParentPair` type).

‎packages/cli/src/commands/migrate/comment-parent-pair.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ async function confirm(question: string): Promise<boolean> {
5555
* unstamped rows are read, so a second run stamps nothing — re-running and
5656
* reading a clean report is the verification. No `sys_migration` flag is
5757
* recorded: nothing gates irreversible behaviour on it. The flags are
58-
* `os migrate comment-parent-pair`'s, one for one.
58+
* `os migrate activity-changed-fields`'s, one for one.
5959
*/
6060
export default class MigrateCommentParentPair extends Command {
6161
static override description =

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

Lines changed: 31 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -333,14 +333,20 @@ function stampCommentParentPair(row: Record<string, unknown>): void {
333333
* `readonly` strip leaves `sys_` objects to their own guards
334334
* (`staticReadonlyInsertSubject`), so this assignment is the guard, as the
335335
* create hook is for `is_edited`.
336-
* - the re-point rule (`beforeUpdate`): an update that names `thread_id` —
337-
* the write the update gate authorizes as an insert into the new thread —
338-
* re-stamps the pair from it. The payload is shared by every row a
336+
* - update (`beforeUpdate`), for every caller: an update that names
337+
* `thread_id` — the re-point the update gate authorizes as an insert into
338+
* the new thread — is stamped from it. The payload is shared by every row a
339339
* predicate update matches, so every row gets the same pair, as it gets the
340-
* same `thread_id`. An update that names no `thread_id` stamps nothing: a
341-
* non-system caller's own pair is stripped by the engine (both columns are
342-
* `readonly`, and the update-side strip keeps only a value a hook
343-
* assigned), and the system's own write is the backfill's.
340+
* same `thread_id`. An update that names either pair column WITHOUT a
341+
* thread is re-derived from the row's own stored thread (`previous`), so no
342+
* writer's value lands, the backfill's included. ⛔ The engine's update-side
343+
* `readonly` strip is not the guard here: under `preserveAudit` (a REST
344+
* 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.
344350
*/
345351
function installCommentParentPairStamp(engine: CommentAccessEngine): void {
346352
engine.registerHook(
@@ -359,9 +365,25 @@ function installCommentParentPairStamp(engine: CommentAccessEngine): void {
359365
(ctx: any) => {
360366
const data: unknown = ctx?.input?.data;
361367
if (!data || typeof data !== 'object' || Array.isArray(data)) return;
362-
if (!Object.prototype.hasOwnProperty.call(data, 'thread_id')) return;
368+
const payload = data as Record<string, unknown>;
369+
const names = (key: string) => Object.prototype.hasOwnProperty.call(payload, key);
370+
if (!names('thread_id') && !COMMENT_PARENT_PAIR_COLUMNS.some(names)) return;
363371
if (!declaresCommentParentPair(engine)) return;
364-
stampCommentParentPair(data as Record<string, unknown>);
372+
if (names('thread_id')) {
373+
stampCommentParentPair(payload);
374+
return;
375+
}
376+
// The pair named without a thread: derive it from the row's own thread.
377+
const previous: unknown = ctx?.previous;
378+
const storedThread =
379+
previous && typeof previous === 'object' ? (previous as Record<string, unknown>).thread_id : undefined;
380+
if (typeof storedThread !== 'string') {
381+
for (const column of COMMENT_PARENT_PAIR_COLUMNS) delete payload[column];
382+
return;
383+
}
384+
const pair = planCommentParentPair(storedThread);
385+
payload[COMMENT_PARENT_OBJECT_COLUMN] = pair.object_name;
386+
payload[COMMENT_PARENT_RECORD_COLUMN] = pair.record_id;
365387
},
366388
{ object: COMMENT_OBJECT, packageId: PACKAGE_ID },
367389
);

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

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,12 @@
2121
* ## As the platform, through the migration door
2222
*
2323
* Rows are read and updated as the platform (a SYSTEM context), the shape of
24-
* `stored-metadata-body-migration.ts` and E6's activity backfill. The update
25-
* names the pair and the row's stored `updated_at` under `preserveAudit`, so
24+
* `stored-metadata-body-migration.ts` and E6's activity backfill. The pair the
25+
* update names is re-derived by the update stamp from the row's thread as it
26+
* is stored at write time (`comment-access-hooks.ts`), so a re-point landing
27+
* between this page's read and the row's write cannot leave the pair naming
28+
* the thread the comment left. The update names the row's stored
29+
* `updated_at` under `preserveAudit`, so
2630
* the backfill changes no comment's `updated_at` and names no `thread_id` or
2731
* `body` (no re-point, no edit stamp). A row whose thread names no record is
2832
* not stamped (its pair stays `null`, and it stays hidden from every

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

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,7 @@ const commentOn = (object: string, id: string, body: string): Row => ({
140140
describe('[ADR-0139 E7] the comment-thread read gate over the stamped parent pair', () => {
141141
let kernel: ObjectKernel;
142142
let engine: ObjectQL;
143+
let driver: SqliteWasmDriver;
143144
const seenWhere: unknown[] = [];
144145
const ids: Record<string, string> = {};
145146

@@ -169,7 +170,7 @@ describe('[ADR-0139 E7] the comment-thread read gate over the stamped parent pai
169170
const atRest = async (id: string): Promise<Row> => (await engine.findOne(COMMENT, { where: { id }, context: SYS })) as Row;
170171

171172
beforeAll(async () => {
172-
({ kernel, engine } = await bootKernel(new SqliteWasmDriver({ filename: ':memory:' })));
173+
({ kernel, engine } = await bootKernel((driver = new SqliteWasmDriver({ filename: ':memory:' }))));
173174
const warehouse = new WarehouseDriver({ filename: ':memory:' });
174175
await warehouse.connect();
175176
engine.registerDriver(warehouse);
@@ -317,7 +318,8 @@ describe('[ADR-0139 E7] the comment-thread read gate over the stamped parent pai
317318
// The state after the upgrade, for three rows: no pair at rest.
318319
const firstComment = ((await engine.find(COMMENT, { where: { thread_id: `${DOC}:${ids.firstDoc}` }, limit: 1, context: SYS })) as Row[])[0];
319320
const rowIds = [String(firstComment.id), ids.onVault, ids.onGatedA];
320-
for (const id of rowIds) await engine.update(COMMENT, { id, object_name: null, record_id: null }, { context: SYS });
321+
// Planted below the engine: every engine write of the pair is re-derived.
322+
for (const id of rowIds) await driver.update(COMMENT, id, { object_name: null, record_id: null });
321323
for (const id of rowIds) expect((await atRest(id)).object_name ?? null).toBeNull();
322324

323325
seenWhere.length = 0;
@@ -352,6 +354,14 @@ describe('[ADR-0139 E7] the comment-thread read gate over the stamped parent pai
352354
// Naming the pair without a thread: the engine strips a member's readonly value.
353355
await engine.update(COMMENT, { object_name: VAULT, record_id: ids.vault, body: 'still mine' }, { where: { id }, context: MEMBER_A });
354356
expect(await atRest(id)).toMatchObject({ body: 'still mine', object_name: DOC, record_id: other });
357+
358+
// ...and under `preserveAudit` (a REST historical import puts it on an
359+
// ordinary caller's write), where the engine's strip keeps a readonly
360+
// value, the stamp re-derives the pair from the row's own thread.
361+
await engine.update(COMMENT, { object_name: VAULT, record_id: ids.vault }, { where: { id }, context: { ...MEMBER_A, preserveAudit: true } });
362+
expect(await atRest(id)).toMatchObject({ thread_id: `${DOC}:${other}`, object_name: DOC, record_id: other });
363+
await engine.update(COMMENT, { record_id: ids.vault }, { where: { id }, context: { ...MEMBER_A, preserveAudit: true } });
364+
expect(await atRest(id)).toMatchObject({ object_name: DOC, record_id: other });
355365
});
356366

357367
it('D5.2: the reaction gate keeps the bounded probe — a literal comment-id list, no leaf', async () => {
@@ -387,17 +397,20 @@ describe('[ADR-0139 E7] backfillCommentParentPair', () => {
387397
const logger = () => ({ info: vi.fn(), warn: vi.fn() });
388398

389399
beforeAll(async () => {
390-
({ kernel, engine } = await bootKernel(new SqliteWasmDriver({ filename: ':memory:' })));
400+
const driver = new SqliteWasmDriver({ filename: ':memory:' });
401+
({ kernel, engine } = await bootKernel(driver));
391402
engine.registry.registerObject(objectOf(DOC) as any, HARNESS_PACKAGE);
392403
await engine.syncSchemas();
393404
for (let i = 0; i < 5; i++) {
394405
const doc = String((await engine.insert(DOC, { name: `d${i}` }, { context: SYS })).id);
395406
rowIds.push(String((await engine.insert(COMMENT, commentOn(DOC, doc, `c${i}`), { context: SYS })).id));
396407
}
397408
rowIds.push(String((await engine.insert(COMMENT, { thread_id: 'free-form', body: 'x', author_id: 'u' }, { context: SYS })).id));
398-
// The state after the upgrade: every row at rest, unstamped, with an updated_at of its own.
409+
// The state after the upgrade: every row at rest, unstamped, with an
410+
// updated_at of its own — planted below the engine, which re-derives any
411+
// pair an engine write names.
399412
for (const id of rowIds) {
400-
await engine.update(COMMENT, { id, object_name: null, record_id: null, updated_at: '2020-01-01T00:00:00.000Z' }, { context: { ...SYS, preserveAudit: true } });
413+
await driver.update(COMMENT, id, { object_name: null, record_id: null, updated_at: '2020-01-01T00:00:00.000Z' });
401414
stampedAt.set(id, (await engine.findOne(COMMENT, { where: { id }, context: SYS }) as Row).updated_at);
402415
}
403416
}, 120_000);

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

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -219,9 +219,9 @@ describe('[ADR-0139 E7] the parent pair: one planner, stamped on create and on t
219219
};
220220
installCommentAccessHooks(engine, () => null, silentLogger());
221221
// The stamp is the one handler that exempts no caller: drive every handler as the system.
222-
const run = async (event: string, data: unknown) => {
222+
const run = async (event: string, data: unknown, previous: unknown = { body: 'b' }) => {
223223
for (const h of registered.filter((r) => r.event === event)) {
224-
await h.handler({ object: 'sys_comment', event, input: { data, options: {} }, session: { isSystem: true }, previous: { body: 'b' } });
224+
await h.handler({ object: 'sys_comment', event, input: { data, options: {} }, session: { isSystem: true }, previous });
225225
}
226226
return data;
227227
};
@@ -249,6 +249,27 @@ describe('[ADR-0139 E7] the parent pair: one planner, stamped on create and on t
249249
expect(await run('beforeUpdate', { body: 'b' })).toEqual({ body: 'b' });
250250
});
251251

252+
it('update: a pair named without a thread is re-derived from the stored thread, or dropped when there is none', async () => {
253+
const run = hooks();
254+
const stored = { body: 'b', thread_id: 'crm_case:c9' };
255+
expect(await run('beforeUpdate', { object_name: 'vault', record_id: 'v1' }, stored)).toEqual({
256+
object_name: 'crm_case',
257+
record_id: 'c9',
258+
});
259+
expect(await run('beforeUpdate', { record_id: 'v1', body: 'x' }, stored)).toMatchObject({
260+
body: 'x',
261+
object_name: 'crm_case',
262+
record_id: 'c9',
263+
});
264+
expect(await run('beforeUpdate', { object_name: null, record_id: null }, { thread_id: 'watercooler' })).toEqual({
265+
object_name: null,
266+
record_id: null,
267+
});
268+
const dropped = (await run('beforeUpdate', { object_name: 'vault', body: 'x' }, undefined)) as Record<string, unknown>;
269+
expect(dropped).toMatchObject({ body: 'x' });
270+
expect(dropped).not.toHaveProperty('object_name');
271+
});
272+
252273
it('a sys_comment that declares no pair is never stamped (no unknown column is written)', async () => {
253274
const run = hooks(false);
254275
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: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -109,9 +109,11 @@ import { ObjectSchema, Field } from '@objectstack/spec/data';
109109
* (the re-point the update gate authorizes as an insert into the new
110110
* thread), the pair is re-stamped from it. A thread naming no record stamps
111111
* `null` / `null`.
112-
* - **Read-only to authors.** Both declare `readonly: true`: a non-system
113-
* update's own value is stripped by the engine; the create side is the
114-
* stamp's assignment.
112+
* - **Read-only to authors.** Both declare `readonly: true`, and no writer's
113+
* value lands: on create the stamp is assigned over it, and an update that
114+
* names either column without a thread has the pair re-derived from the
115+
* row's stored thread — for every caller, because under `preserveAudit` (a
116+
* historical import) the engine's update-side strip keeps a readonly value.
115117
* - **Stored rows.** Rows written before the pair existed carry `null` until
116118
* `os migrate comment-parent-pair --apply` stamps them; until then the read
117119
* gate answers them with the bounded thread probe (fail closed), never a

‎packages/qa/dogfood/test/comment-thread-parent-pair.dogfood.test.ts‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -279,7 +279,10 @@ describe('[ADR-0139 E7] the comment-thread gate leafs over the parent pair: ever
279279

280280
it('a row the backfill has not reached is probed by its thread, not leafed: still served when readable, still hidden when not', async () => {
281281
const cleared = [...bulk.mine.slice(0, UNSTAMPED), ...bulk.other.slice(0, UNSTAMPED)];
282-
for (const rowId of cleared) await ql.update(COMMENT, { id: rowId, object_name: null, record_id: null }, { context: { ...SYS } });
282+
// Planted below the engine, as a row from before the upgrade is: every
283+
// engine write of the pair is re-derived from the row's thread.
284+
const driver = ql.getDriver(COMMENT);
285+
for (const rowId of cleared) await driver.update(COMMENT, rowId, { object_name: null, record_id: null });
283286
try {
284287
commentReads.length = 0;
285288
const { ids, totals } = await walk('member', bothThreads());
@@ -289,11 +292,10 @@ describe('[ADR-0139 E7] the comment-thread gate leafs over the parent pair: ever
289292
expect(probes.length).toBeGreaterThan(0);
290293
expect(probes.every((r) => r.where.includes('"object_name":{"$null":true}'))).toBe(true);
291294
} finally {
292-
for (const rowId of cleared) {
293-
const row = await ql.findOne(COMMENT, { where: { id: rowId }, context: { ...SYS } });
294-
const [object, recordId] = String(row.thread_id).split(':');
295-
await ql.update(COMMENT, { id: rowId, object_name: object, record_id: recordId }, { context: { ...SYS } });
296-
}
295+
// The engine write re-derives each row's pair from its thread: the backfill's own write path.
296+
for (const rowId of cleared) await ql.update(COMMENT, { id: rowId, object_name: null, record_id: null }, { context: { ...SYS } });
297+
const restored: Row[] = await ql.find(COMMENT, { where: { id: { $in: cleared } }, context: { ...SYS } });
298+
expect(restored.every((r) => r.thread_id === `${r.object_name}:${r.record_id}`)).toBe(true);
297299
}
298300
});
299301
});

0 commit comments

Comments
 (0)