Skip to content

Commit dc93781

Browse files
feat(plugin-audit): the platform stamps sys_comment is_edited / edited_at when a comment's body is edited (#23153)
Fixes #23124 Clause-②: no (narrowing) — it narrows a published accept set: a client's write to `is_edited` / `edited_at` no longer takes effect. Per the ruling, it owes one contract-review-tier review before the queue. ## What this does Executes maintainer ruling A (recorded in comment 6137155820): the platform, not the console, marks an edited comment. - **The stamp.** `sys_comment` gets a second `beforeUpdate` handler, `stampCommentBodyEdit` in `comment-access-hooks.ts`. It sets `is_edited: true` and `edited_at` (server clock, ISO string) when the update's `body` differs from the stored body (`ctx.previous.body`). An update that does not name `body`, or sends the stored text again beside new `mentions` (the console's shape), stamps nothing. It runs for every caller, system included, the way the engine's `updated_at` stamp does. Under `preserveAudit` (historical import) a supplied value is kept, again as `updated_at` does. It is registered on its own, not inside the authorization handler, because that handler returns early for system and context-less callers and declares the whole-operation dispatch. - **Read-only at the data door.** `is_edited` and `edited_at` declare `readonly: true`. - **Create.** The engine leaves `sys_` objects to their own guards on create (`staticReadonlyInsertSubject`), so the comment create hook sets both fields to "not edited" when a client sends them. This works the same way as the `author_id` stamp next to it. - **Docs in code.** The `sys_comment` docblock records the decision (ADR-0049, declared = enforced), in the same voice as its two earlier entries. The module header of `comment-access-hooks.ts` points to the stamp. ## Mechanism hypotheses, measured The rig: base `1ca7a805c`, a real kernel with ObjectQL, AuditPlugin and the SQLite driver. The caller is the comment's author. **H1 holds.** `git grep -n "is_edited\|edited_at"` outside the generated translation bundles returns 2 hits, the two declarations in `sys-comment.object.ts`. A body edit stored `is_edited: false` and `edited_at: null`. The author's edit already passes the existing authorization `beforeUpdate`. **H2: what the engine does today, measured on the `created_at` precedent.** | write (as the author) | stored | reported | |:--|:--|:--| | update `{ body, created_at: forged }` | body written, `created_at` unchanged | `onFieldsDropped` reported `{ fields: [created_at], reason: readonly }`. This is the REST PATCH's `droppedFields`, plus a `warn` line | | the same, with `strictReadonlyWrites: true` | nothing | `ERR_READONLY_FIELD_REJECTED` thrown. This is an in-process opt-in, and no transport passes it | | create `{ created_at: forged }` | the server time (the engine's audit stamp overwrites the value) | nothing | | update `{ is_edited: true, edited_at: forged }`, before this change | both stored | nothing | | create carrying both, before this change | both stored | nothing | **Choice: (a) and (b) together.** On update, `readonly: true` gives the engine's strip-and-report, and the stamp's overwrite wins on an edit. On create, the comment create hook overwrites. There is no `packages/spec` change. There is no loud refusal, for two reasons: - The engine does not offer one at the data door. Its strict mode is an in-process opt-in on a single call. - A refusal written in the comment hook would reject the edit the released console makes. The console at the pinned `.objectui-sha` (`1fb1ced`) and on objectui main (`5d3f437`) sends `is_edited: true, edited_at: CLIENT_CLOCK` with every edit (`handleEditComment` in `RecordDetailView.tsx`). The ruling files the console's removal only once this release is installable, so a refusal would break comment editing in the shipped console for that whole interval. The strip is not silent at the REST door: it shows up in `droppedFields` and in a `warn` line. The one silent case is a client value sent together with a body edit. There the stamp replaces it with the true value, the same shape as `created_at` / `updated_at`. **H3: no bypass.** A `multi: true` predicate update dispatches `beforeUpdate` once per matched row, with `input.id` and `previous` bound. Measured: 2 matched rows gave 2 dispatches, each carrying its own row's prior body. What that means for the stamp, all pinned: - If every row's body changes, every row is stamped. - If no row's body changes, no row is stamped. - If the rows disagree, the engine refuses the whole batch (`MULTI_UPDATE_HOOK_KEY_DIVERGENCE`, 400, nothing written). One SET clause cannot mark some rows and not others. Other doors checked: - The engine has no upsert path; `update.options.upsert` is a retired tombstone. - `insert` / `insertMany` dispatch `beforeInsert` once per row. - REST batch, protocol `updateData` / `updateManyData`, the import runner and the automation CRUD nodes all go through `engine.update`. - The only door that reaches a table without hooks is raw `engine.execute()`, a driver statement. It bypasses every hook and every strip for every object by design, no transport exposes it, and its only callers in the tree are migrations. No caller writes `sys_comment` through it. So there is no fork. ## Pins `comment-edit-stamp.integration.test.ts` has 10 tests on the real engine: 1. An author edit of the body stamps both fields from the server clock. 2. A `mentions`-only edit does not stamp, with or without the unchanged body sent beside it. 3. A create does not stamp, and a create carrying `is_edited: true, edited_at: forged` stores "not edited". 4. A body edit carrying `is_edited: false, edited_at: forged` is stamped `true` with the server clock. 5. With no edit, `{ is_edited: true, edited_at: forged }` and later `{ is_edited: false, edited_at: null }` change nothing. Each is reported as `{ reason: readonly, fields: [is_edited, edited_at] }`. 6. A predicate update where every body changes stamps every row. 7. A predicate update where no body changes stamps no row. 8. A predicate update whose rows disagree is refused: `mapDataError` gives 400 `MULTI_UPDATE_HOOK_KEY_DIVERGENCE`, the error's `keys` are `[edited_at, is_edited]`, and nothing is written. 9. A system-context body edit is stamped. 10. A `preserveAudit` update keeps the mark it supplies. **Run against the base's implementation** (both source files restored from `1ca7a805c`, test file unchanged): `Tests 7 failed | 3 passed (10)`. The 3 that pass are 2, 7 and 10. They pass there by construction, because nothing was stamped and nothing was read-only, so they carry no weight alone. The no-stamp pins 2 and 7 are held by the ablations below. ## Ablations Each ran through `scripts/ablation-replace.mjs` in wrap mode, after the implementation was committed. The anchor hit was verified on disk, and each restore was proven: blob equal to HEAD and `git diff HEAD` empty. | # | forbidden behaviour put back | red | |:--|:--|:--| | A1 | delete the stamp's `body`-present guard | pin 2 (`an update that does not name body stamped the comment`), pin 5 | | A2 | delete the unchanged-body comparison | pin 2 (`an update re-sending the stored body stamped the comment`), pin 7 (`unchanged row … was stamped`), pin 8 (`a mixed predicate update was accepted`) | | A3 | both fields declared `readonly: false` | pin 5 (`a client marked an unedited comment edited: expected { is_edited: true, …(1) } to deeply equal { is_edited: false, edited_at: null }`) | | A4 | the stamp prefers the client's submission (`ctx.submitted.* ?? stamp`) | pin 4 (`the client cleared the mark on its own edit: expected false to be true`) | | A5 | delete the create-side overwrite | pin 3 (`a client-supplied mark landed on create`) | A3's first attempt used a replacement that was a substring of its anchor. The tool refused it (the replacement count could not rise), so it never ran. It was re-run with `readonly: false` and went red as shown. ## Tests All on HEAD `67900682e2`: - `pnpm --filter @objectstack/plugin-audit test`: `Test Files 48 passed (48)`, `Tests 771 passed (771)`. - `pnpm --filter @objectstack/plugin-audit typecheck`: exit 0, including `check:test-typecheck: OK`. `tsc --listFiles` over `tsconfig.test.json` lists both edited test files. - `pnpm --filter @objectstack/plugin-audit build`: exit 0. ## Gates The gate list comes from `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack`, run with no paths at HEAD `67900682e2`. It gives 67 commands, the same as the PM's list for tree `1ca7a805c`. All 67 were run at `67900682e2`: - **66 exit 0.** - `pnpm check:i18n` first exited 3 (PREREQUISITE NOT MET: the CLI was not built). After the gate's own one-command prerequisite build it reads `check-i18n-bundles: OK (10 package(s) — all bundles in sync, no undeclared authoring keys)`, and plugin-audit's 7 bundles are in sync. - `pnpm check:dual-build-cjs-loads` is **NOT MEASURED**. It exited 3, PREREQUISITE NOT MET, because it reads every package's built output and 54 packages have no `dist/` in this worktree. It needs a whole-repo `pnpm build`, so it is left to CI. plugin-audit's own build passes `check-dts-emitted`, and its exports are unchanged. `dispatch-gates --ran`, with an exit code recorded for every command: 67 derived, 66 run, 1 NOT-MEASURED (derived from the recorded exit 3), 0 UNRUN. Verdict lines quoted from the gates: - `check-adr-0087-registration`: `✓ 1 declared-breaking changeset(s), each carrying an ADR-0087 disposition`, reading `[BREAKING+clause-②-narrowing] not-required (no-migration-prescription)`. - `check-changeset-no-major`: `✓ This diff introduces no major bump`. - `check-empty-changeset`: `✓ No empty-frontmatter changeset introduced by this diff`. - `check:nul-bytes`: `OK … no raw ASCII control bytes`. - `check:test-source-alias`: `OK`. **Lint, narrowed and stated as a measurement:** - `pnpm exec eslint --no-inline-config --format json` on the 4 changed `.ts` files: 4 files, 0 errors, 0 warnings. - Each file is inside eslint's own population (`ESLint.isPathIgnored` returns false for all 4). - `eslint.config.mjs` never enables type-aware linting (no `parserOptions.project`), so this diff cannot move the verdict of any file it does not touch. - The full `pnpm lint` run belongs to CI. ## Acceptance notes - **File surface.** As claimed: the hooks file, the object file, their tests and one changeset. `audit-plugin.ts` is untouched, and no translation bundle changed, since no label or description changed. Two edits to the existing `comment-access-hooks.test.ts` are harness changes, not new pins. Its fake engine kept the last handler per event, so the stamp's registration would have replaced the authorization handler those tests drive. The harness now keeps the first handler per event. Its wired `sys_comment` fixture now declares the two fields the stamp writes, because the engine refuses undeclared keys. - **Clause-② arm.** The claim's `Clause-②` line, copied above as written, carries no direction arm, but its reasoning names a narrowing. The changeset declares `Clause-②: no (narrowing)` together with the **BREAKING** banner and an ADR-0087 `not-required (no-migration-prescription)` marker, and `check-adr-0087-registration` reads it as `[BREAKING+clause-②-narrowing]`. If line 2 of this body should carry the arm as well, the edit is to insert ` (narrowing)` after `no`. - **Changeset level.** `minor`, under the launch-window convention for accept-set narrowings. The contract review judges the level. - **objectui half.** Not touched here. Once this release is installable, the console's two-line write in `handleEditComment` becomes inert: its values are stamped over on an edit and stripped otherwise. - **Observation, not filed.** `mapDataError` maps a `ReadonlyFieldRejectedError` (the `strictReadonlyWrites` refusal, which carries no `status`) to `404 OBJECT_NOT_FOUND`. This was measured in-process only. No transport passes `strictReadonlyWrites`, so nothing public reaches it. --- _Generated by [Claude Code](https://claude.ai/code/session_01FzQy26KznSa5j2gnf3AzsE)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 02bec6c commit dc93781

5 files changed

Lines changed: 416 additions & 1 deletion

File tree

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
---
2+
'@objectstack/plugin-audit': minor
3+
---
4+
5+
`sys_comment` edit marks are platform-written: an update that changes a comment's `body` stamps `is_edited: true` and `edited_at` from the server clock, and a client can no longer set or clear either field.
6+
7+
Clause-②: no (narrowing)
8+
9+
<!-- adr-0087: not-required (no-migration-prescription) No metadata moves: `sys_comment` keeps both fields with their names and types, no spec key, authorable spelling, export or stored shape is added, removed, renamed or re-shaped, and a row stored before this change keeps its bytes, so there is nothing for `objectstack migrate meta` to rewrite. What narrows is one runtime write door: a client's value for two platform-written fields no longer lands, and a predicate body update whose matched rows disagree is refused; the remedy is in the request a caller sends to that door, never a rewrite of a package source or a stored row. The other categories are closed on facts: the package publishes (not unpublished); no ADR-0087 id is named or touched (not registered or already-registered); and no exported declaration is removed or narrowed (not runtime-interface-only, type-surface-only or runtime-export-retired). -->
10+
11+
**BREAKING** (an accept-set narrowing), shipped as `minor` under the launch-window convention for breaking changes.
12+
13+
- **Stamped by the platform.** The comment's `beforeUpdate` sets both fields when the update's `body` differs from the stored body, for every caller, the way the engine stamps `updated_at`. An update that does not name `body`, or sends the stored text again beside new `mentions`, is not an edit and stamps nothing. A create is not an edit either.
14+
- **Read-only at the data door.** `is_edited` and `edited_at` now declare `readonly: true`. On an update that edits the body, the stamp replaces whatever the client sent; on any other update a non-system caller's value is dropped and reported in the response's `droppedFields`; on a create the platform sets both to "not edited" over a client value. Nothing is refused for carrying the fields — a client that still sends them on every edit, as the console released before this change does, keeps working and its values are ignored. Read both fields back from the row instead of writing them.
15+
- **Predicate updates.** A `multi: true` update is stamped per matched row. One whose matched rows disagree — some bodies change, some already hold the new text — is now refused whole with `MULTI_UPDATE_HOOK_KEY_DIVERGENCE` (400) and writes nothing, because one SET clause cannot mark some rows and not others. Narrow the predicate to the rows whose text changes, or update by id.
16+
- A `preserveAudit` (historical import) update keeps a mark it supplies, as it keeps `updated_at`.

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

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,8 +71,11 @@ function install(opts: {
7171
const parentReads: Array<{ object: string; options: any }> = [];
7272
const parentFind = readableParentFind(opts.readable ?? [], parentReads);
7373
const engine: CommentAccessEngine = {
74+
// The FIRST handler per event is the authorization gate these tests drive:
75+
// `beforeUpdate` also carries the edit stamp (#23124), registered after it
76+
// and pinned on the real engine in `comment-edit-stamp.integration.test.ts`.
7477
registerHook: (event, handler) => {
75-
hooks.set(event, handler as any);
78+
if (!hooks.has(event)) hooks.set(event, handler as any);
7679
},
7780
find: async (object, options: any) => {
7881
if (object !== 'sys_comment') return parentFind(object, options);
@@ -644,6 +647,10 @@ const COMMENT_FIELDS = {
644647
thread_id: { name: 'thread_id', label: 'Thread', type: 'text' as const },
645648
author_id: { name: 'author_id', label: 'Author', type: 'text' as const },
646649
body: { name: 'body', label: 'Body', type: 'text' as const },
650+
// The edit stamp `installCommentAccessHooks` also registers (#23124) writes
651+
// these on a body change; declared as `sys_comment` declares them.
652+
is_edited: { name: 'is_edited', label: 'Edited', type: 'boolean' as const, readonly: true },
653+
edited_at: { name: 'edited_at', label: 'Edited At', type: 'datetime' as const, readonly: true },
647654
};
648655
const sysCommentObject = { name: 'sys_comment', label: 'Comment', fields: COMMENT_FIELDS };
649656
const crmOpportunityObject = {

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

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,10 @@
3939
* platform's not-visible refusal instead of the named one (#21755); see
4040
* {@link refuseNotVisible}. The named refusal is what a caller who can
4141
* read the parent, but may not edit it, receives.
42+
* * The edit stamp (#23124, ruling A): a SECOND `beforeUpdate` handler, not
43+
* part of the authorization above, sets `is_edited` / `edited_at` when the
44+
* body changes — see {@link stampCommentBodyEdit}. The create hook sets
45+
* both to "not edited" over a client value.
4246
* - {@link installCommentReadVisibility} — the read side: a
4347
* `find`/`findOne`/`count`/`aggregate` middleware that intersects the query
4448
* with the threads whose parent record the caller can actually read.
@@ -319,6 +323,59 @@ function refuseNotVisible(
319323
throw err;
320324
}
321325

326+
/**
327+
* [#23124, maintainer ruling A] The platform marks a comment edited: when an
328+
* update's `body` differs from the stored body, set `is_edited: true` and
329+
* `edited_at` to the server clock. Both fields are `readonly` on
330+
* `sys_comment`, so on an update by a non-system caller this hook is their one
331+
* writer, as the create hook is on a create (`objects/sys-comment.object.ts`,
332+
* "Platform-written").
333+
*
334+
* What counts as an edit is the BODY and nothing else: an update that does not
335+
* name `body`, or names the stored text again (the console rewrites `mentions`
336+
* beside it), stamps nothing. A row with no readable pre-image is stamped —
337+
* the engine binds `previous` on both update paths, and a mark that cannot be
338+
* proven unneeded is the safe side of a reader's trust signal.
339+
*
340+
* Every caller is stamped, system included, the way the engine's `updated_at`
341+
* stamp is: the mark records that the text changed after it was posted, not
342+
* who was allowed to change it (the authorization handler is separate and
343+
* exempts system callers; this one does not). The one exception is the
344+
* engine's own: a `preserveAudit` write (a historical import) keeps a value it
345+
* supplies, as `updated_at` does, and is stamped only where it supplies none.
346+
*
347+
* Why the stamp is an assignment and the client's value needs no handling here:
348+
* the engine hides a non-system caller's `readonly` values from the hooks and
349+
* strips them after, and keeps a value a hook ASSIGNED (`hookWrittenKeys`). So
350+
* on an edit the stamp lands whatever the client sent, and without one the
351+
* client's value is dropped and reported as `droppedFields`.
352+
*
353+
* A predicate (`multi: true`) update dispatches this handler once per matched
354+
* row, with that row as `previous`, over ONE shared payload. If every row's
355+
* body changes, every row is stamped; if none does, none is. A batch whose rows
356+
* disagree is refused whole by the engine (`MULTI_UPDATE_HOOK_KEY_DIVERGENCE`,
357+
* 400, nothing written) — one SET clause cannot mark some rows and not others,
358+
* and stamping all of them would mark text that never changed.
359+
*/
360+
async function stampCommentBodyEdit(ctx: any): Promise<void> {
361+
const data: unknown = ctx?.input?.data;
362+
if (!data || typeof data !== 'object' || Array.isArray(data)) return;
363+
const payload = data as Record<string, unknown>;
364+
if (!Object.prototype.hasOwnProperty.call(payload, 'body')) return;
365+
const previous: unknown = ctx?.previous;
366+
if (previous && typeof previous === 'object' && Object.is((previous as Record<string, unknown>).body, payload.body)) {
367+
return;
368+
}
369+
const now = new Date().toISOString();
370+
if (ctx?.session?.preserveAudit === true) {
371+
payload.is_edited = payload.is_edited ?? true;
372+
payload.edited_at = payload.edited_at ?? now;
373+
return;
374+
}
375+
payload.is_edited = true;
376+
payload.edited_at = now;
377+
}
378+
322379
function asIdList(id: unknown): Array<string | number> | null {
323380
if (typeof id === 'string' || typeof id === 'number') return [id];
324381
if (id && typeof id === 'object' && Array.isArray((id as any).$in)) {
@@ -539,6 +596,14 @@ export function installCommentAccessHooks(
539596
// spoofable — a caller could post as anyone, then "author-delete" it.
540597
if (ctx.session.userId) data.author_id = ctx.session.userId;
541598

599+
// [#23124] A new comment is not an edited one, whatever the client sent.
600+
// The engine's create-side `readonly` strip leaves `sys_` objects to their
601+
// own guards, so this is the guard: the platform's value, assigned over a
602+
// client's, exactly as `author_id` above. Only a key the client named is
603+
// touched; an absent one takes the field's default.
604+
if (Object.prototype.hasOwnProperty.call(data, 'is_edited')) data.is_edited = false;
605+
if (Object.prototype.hasOwnProperty.call(data, 'edited_at')) data.edited_at = null;
606+
542607
const target = parseCommentThreadId(data.thread_id);
543608
if (!target) {
544609
forbid(
@@ -729,6 +794,13 @@ export function installCommentAccessHooks(
729794
{ object: 'sys_comment', packageId: PACKAGE_ID, dispatchUnscopedMultiWrite: true },
730795
);
731796

797+
// ── Update: the platform stamps an edit of the body (#23124) ─────────
798+
// Its own registration, deliberately: the authorization handler above
799+
// returns early for system and context-less callers and declares the
800+
// whole-operation dispatch, and the stamp wants neither — see
801+
// {@link stampCommentBodyEdit}.
802+
engine.registerHook('beforeUpdate', stampCommentBodyEdit, { object: COMMENT_OBJECT, packageId: PACKAGE_ID });
803+
732804
// ── Delete: author or parent editor ─────────────────────────────────
733805
engine.registerHook(
734806
'beforeDelete',

0 commit comments

Comments
 (0)