diff --git a/.changeset/sys-comment-retire-visibility-reply-count.md b/.changeset/sys-comment-retire-visibility-reply-count.md new file mode 100644 index 0000000000..6a5a1a8120 --- /dev/null +++ b/.changeset/sys-comment-retire-visibility-reply-count.md @@ -0,0 +1,68 @@ +--- +"@objectstack/plugin-audit": minor +--- + +feat(plugin-audit)!: retire `sys_comment.visibility` and `sys_comment.reply_count` (#4756, ADR-0049) + +Both fields were modelled with **zero** runtime consumers — nothing in this repo, +in `objectui`, or in `cloud` ever read or maintained either one. ADR-0049 +enforce-or-remove; maintainer decision: remove both. Same disposition, and for +the same stated reason, as `sys_attachment.share_type` / `sys_attachment.visibility` +in #2755 ("attachment access is derived from the parent record"). + +**REMOVED — `sys_comment.visibility`** (`'public' | 'internal' | 'private'`, +defaulted `'public'`). + +This one is a **security-looking key with no gate behind it**, which is the +primary reason it goes rather than stays. No code path consulted it: not +`enforceFeedsCapability`, not the record-level gates added in #4630, not the +REST layer, not objectui's discussion panel. A comment an author marked +`private` was visible to exactly the same people as a `public` one — an app +author (or an AI authoring metadata) reading the field list would reasonably +believe otherwise, and get a silent security failure instead of an error. That +is the Prime Directive #10 trap in its textbook shape. + +There is **no replacement key**: after #4630, who can see a comment is decided +by the record-level permissions of the record its `thread_id` names — one +coherent rule. A per-row enum layered on top would be a second source of truth +for the same question. The enum's only genuinely missing meaning ("hidden from +external/portal principals") depends on external principals existing at all, +which waits on ADR-0090 D11's `externalSharingModel`; today there is nobody to +hide a comment from. This does not foreclose that design — when portals land, +a visibility key can return **enforce-first**, with a real gate and tests. + +**FROM → TO:** stop sending `visibility` on `sys_comment` writes; to restrict +who sees a discussion, restrict who can read the record `thread_id` points at. + +**REMOVED — `sys_comment.reply_count`** (`number`, `defaultValue: 0`, +`readonly: true`). + +Never incremented anywhere, and `readonly` meant an author could not set it by +hand either, so every row read `0` forever — a UI binding an "N replies" badge +to it rendered `0` for every thread. Deliberately **not** replaced by an +`afterInsert`/`afterDelete` roll-up: the predicate/bulk write-hook gaps tracked +by #4770 / #4778 / #4779 (a hook that returns early without a single-record id +lets the whole bulk operation through) are exactly where a hook-maintained +counter drifts — a bulk delete of replies would never decrement it. A counter +that drifts is worse than no counter, because both the UI and an AI reading the +record trust it. If a badge needs the number, aggregate `parent_id` children at +read time; a designed roll-up can be revisited once #4775's family has settled +bulk-hook semantics. + +**FROM → TO:** replace reads of `reply_count` with a count of `sys_comment` rows +whose `parent_id` is the comment's id. + +**Stored data.** Existing databases keep both columns as **unmanaged leftovers** +— no migration, matching #2755. What changes where: + +- **Reads are loud everywhere.** The read-axis gates (#4134 / #4226) resolve + field names from the object schema, not from the table, so a filter, sort, + `select` or `expand` naming `visibility` / `reply_count` now answers + `400 INVALID_FIELD` on every deployment, leftover column or not. A "0 replies" + badge that silently lied becomes an error that names itself. +- **Writes are loud on new databases only.** A database provisioned after this + change has no such column, so the write fails at the driver and is mapped to + the same `400 INVALID_FIELD` envelope. On a pre-existing database the leftover + column still accepts a value nothing will ever read — record validation does + not reject undeclared keys. Dropping the two columns is an optional manual + cleanup, not a requirement. diff --git a/docs/adr/0052-audit-is-not-the-activity-feed.md b/docs/adr/0052-audit-is-not-the-activity-feed.md index 3df1128bd7..6af69eeb6a 100644 --- a/docs/adr/0052-audit-is-not-the-activity-feed.md +++ b/docs/adr/0052-audit-is-not-the-activity-feed.md @@ -177,6 +177,16 @@ timeline. But weighing it against the implementation reality reversed that lean: | REST | ✅ generic data API | ❌ nested `/data/{obj}/{id}/feed` route unmounted (404) | | threads/mentions/reactions | ✅ fields already declared (`parent_id`, `reply_count`, `mentions`, `reactions`) | ✅ (but unreachable) | +> **Note (#4756).** The row above records the field list **as it stood at +> decision time**, and the decision itself is unchanged. One of those fields has +> since been retired: `sys_comment.reply_count` was declared but never +> incremented by anything, so it read `0` on every row forever — removed under +> ADR-0049 enforce-or-remove, with a reply count taken as an aggregate over +> `parent_id` children at read time. Threading itself (`parent_id`), mentions and +> reactions are unaffected. `sys_comment.visibility` went with it in the same +> change (never consulted by any gate; comment visibility derives from the record +> `thread_id` names, #4630). + Picking the durable, default, UI-wired system reaches "one backend" **now**, at near-zero risk. `service-feed`'s only real edge — one unified *typed* stream — is obtained on the chosen family by treating **`sys_activity` as the unified diff --git a/packages/plugins/plugin-audit/src/objects/sys-comment-retired-fields.test.ts b/packages/plugins/plugin-audit/src/objects/sys-comment-retired-fields.test.ts new file mode 100644 index 0000000000..ec9f3650b1 --- /dev/null +++ b/packages/plugins/plugin-audit/src/objects/sys-comment-retired-fields.test.ts @@ -0,0 +1,74 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { SysComment } from './index.js'; + +/** + * #4756 — `sys_comment.visibility` / `sys_comment.reply_count` are RETIRED + * (ADR-0049 enforce-or-remove). Both were declared with zero runtime consumers + * in this repo, in `objectui` and in `cloud`; the removal follows the + * `sys_attachment.share_type` / `sys_attachment.visibility` precedent (#2755). + * + * This file exists so the removal cannot be undone by accident. Re-declaring + * either field turns it red, and the failure message carries the prescription + * — that is the whole point: an object field has no `retiredKey()` tombstone to + * reject the name at authoring time the way a spec property does, so the pin + * IS the tombstone for the platform-owned declaration. + * + * If a future change genuinely needs one of these names back, it arrives + * enforce-first — with the gate/roll-up and its own tests — and updates this + * file deliberately, never as collateral of an unrelated edit. + */ + +const RETIRED_FIELDS: ReadonlyArray = [ + [ + 'visibility', + 'comment visibility derives from the record `thread_id` points at (#4630); ' + + 'a per-row enum would be a second, unenforced source of truth. An external/' + + 'portal distinction must be designed against ADR-0090 D11 `externalSharingModel` first.', + ], + [ + 'reply_count', + 'count `parent_id` children at read time (#4756); a hook-maintained roll-up ' + + 'drifts through the predicate/bulk write-hook gaps tracked by #4770 / #4778 / #4779.', + ], +]; + +describe('sys_comment — retired fields stay retired (#4756)', () => { + it.each(RETIRED_FIELDS)( + '%s is not declared on sys_comment', + (field, prescription) => { + const fields = (SysComment as { fields?: Record }).fields ?? {}; + expect( + Object.keys(fields), + `sys_comment.${field} was retired under ADR-0049 (#4756) — ${prescription}`, + ).not.toContain(field); + }, + ); + + it.each(RETIRED_FIELDS)( + '%s is not referenced by an index, highlight or title declaration either', + (field) => { + const object = SysComment as { + indexes?: Array<{ fields?: string[] }>; + highlightFields?: string[]; + titleFormat?: string; + nameField?: string; + displayNameField?: string; + }; + const indexedFields = (object.indexes ?? []).flatMap((i) => i.fields ?? []); + expect(indexedFields).not.toContain(field); + expect(object.highlightFields ?? []).not.toContain(field); + expect(object.titleFormat ?? '').not.toContain(field); + expect([object.nameField, object.displayNameField]).not.toContain(field); + }, + ); + + it('keeps the replacement paths both prescriptions point at', () => { + const fields = (SysComment as { fields?: Record }).fields ?? {}; + // `reply_count` → aggregate over children of `parent_id`. + expect(Object.keys(fields)).toContain('parent_id'); + // `visibility` → the record-level permissions of what `thread_id` names. + expect(Object.keys(fields)).toContain('thread_id'); + }); +}); diff --git a/packages/plugins/plugin-audit/src/objects/sys-comment.object.ts b/packages/plugins/plugin-audit/src/objects/sys-comment.object.ts index 847d361341..c5c85c2604 100644 --- a/packages/plugins/plugin-audit/src/objects/sys-comment.object.ts +++ b/packages/plugins/plugin-audit/src/objects/sys-comment.object.ts @@ -15,6 +15,34 @@ import { ObjectSchema, Field } from '@objectstack/spec/data'; * `sys_comment` when you want a focused threaded discussion surface * without the heavier Chatter envelope. * + * ## Removed fields (ADR-0049 enforce-or-remove, #4756) + * + * Two fields were modelled here with **zero** runtime consumers — nothing in + * this repo, in `objectui`, or in `cloud` ever read or maintained them. Under + * ADR-0049 a declared-but-unenforced key is removed rather than left to lie, + * following the `sys_attachment.share_type` / `sys_attachment.visibility` + * precedent (#2755: "attachment access is derived from the parent record"). + * + * - **`visibility`** (`'public' | 'internal' | 'private'`) — never consulted by + * any gate: not `enforceFeedsCapability`, not the record-level gates added in + * #4630, not the REST layer, not objectui's discussion panel. A comment marked + * `private` was exactly as visible as a `public` one — a *security-looking* + * lever with no gate behind it (Prime Directive #10). **Prescription:** comment + * visibility is decided by the record-level permissions of the record + * `thread_id` names (#4630) — there is no per-row override. A designed + * external/portal distinction would have to be defined against ADR-0090 D11's + * `externalSharingModel` first, and can return as an enforced key with tests. + * - **`reply_count`** — never incremented; `readonly: true` meant an author + * could not even set it by hand, so every row read `0` forever. **Prescription:** + * count `parent_id` children at read time. Deliberately NOT re-introduced as a + * hook-maintained roll-up: the predicate/bulk write-hook gaps tracked by + * #4770 / #4778 / #4779 are exactly where such a counter drifts (a bulk delete + * of replies would never decrement it), and a counter that drifts is worse + * than no counter — both the UI and an AI reading the record would trust it. + * + * Existing databases keep the two columns as unmanaged leftovers; there is no + * migration (same disposition as #2755). + * * @namespace sys */ export const SysComment = ObjectSchema.create({ @@ -49,6 +77,8 @@ export const SysComment = ObjectSchema.create({ group: 'Thread', }), + // The reply relationship is `parent_id` and nothing else — a reply count is + // an aggregate over the children, computed at read time (#4756). parent_id: Field.lookup('sys_comment', { label: 'Parent Comment', required: false, @@ -56,13 +86,6 @@ export const SysComment = ObjectSchema.create({ group: 'Thread', }), - reply_count: Field.number({ - label: 'Reply Count', - defaultValue: 0, - readonly: true, - group: 'Thread', - }), - // ── Author ─────────────────────────────────────────────────── author_id: Field.lookup('sys_user', { label: 'Author', @@ -119,14 +142,8 @@ export const SysComment = ObjectSchema.create({ group: 'Lifecycle', }), - visibility: Field.select( - ['public', 'internal', 'private'], - { - label: 'Visibility', - defaultValue: 'public', - group: 'Lifecycle', - }, - ), + // No `visibility` field: who can see a comment is decided by the record + // `thread_id` points at (#4630, #4756) — one rule, no second source. created_at: Field.datetime({ label: 'Created At', diff --git a/packages/plugins/plugin-audit/src/translations/en.objects.generated.ts b/packages/plugins/plugin-audit/src/translations/en.objects.generated.ts index 5476129bc6..c1d4501c63 100644 --- a/packages/plugins/plugin-audit/src/translations/en.objects.generated.ts +++ b/packages/plugins/plugin-audit/src/translations/en.objects.generated.ts @@ -186,9 +186,6 @@ export const enObjects: NonNullable = { label: "Parent Comment", help: "Optional parent comment for nested replies" }, - reply_count: { - label: "Reply Count" - }, author_id: { label: "Author" }, @@ -216,14 +213,6 @@ export const enObjects: NonNullable = { edited_at: { label: "Edited At" }, - visibility: { - label: "Visibility", - options: { - public: "public", - internal: "internal", - private: "private" - } - }, created_at: { label: "Created At" }, diff --git a/packages/plugins/plugin-audit/src/translations/es-ES.objects.generated.ts b/packages/plugins/plugin-audit/src/translations/es-ES.objects.generated.ts index 665986bffe..4467271cbe 100644 --- a/packages/plugins/plugin-audit/src/translations/es-ES.objects.generated.ts +++ b/packages/plugins/plugin-audit/src/translations/es-ES.objects.generated.ts @@ -186,9 +186,6 @@ export const esESObjects: NonNullable = { label: "Comentario principal", help: "Comentario principal opcional para respuestas anidadas." }, - reply_count: { - label: "Número de respuestas" - }, author_id: { label: "Autor" }, @@ -216,14 +213,6 @@ export const esESObjects: NonNullable = { edited_at: { label: "Editado el" }, - visibility: { - label: "Visibilidad", - options: { - public: "Público", - internal: "Interno", - private: "Privado" - } - }, created_at: { label: "Creado el" }, diff --git a/packages/plugins/plugin-audit/src/translations/ja-JP.objects.generated.ts b/packages/plugins/plugin-audit/src/translations/ja-JP.objects.generated.ts index 80d66b1e98..c02ec175ac 100644 --- a/packages/plugins/plugin-audit/src/translations/ja-JP.objects.generated.ts +++ b/packages/plugins/plugin-audit/src/translations/ja-JP.objects.generated.ts @@ -186,9 +186,6 @@ export const jaJPObjects: NonNullable = { label: "親コメント", help: "ネストした返信用のオプションの親コメント" }, - reply_count: { - label: "返信数" - }, author_id: { label: "投稿者" }, @@ -216,14 +213,6 @@ export const jaJPObjects: NonNullable = { edited_at: { label: "編集日時" }, - visibility: { - label: "公開範囲", - options: { - public: "公開", - internal: "内部", - private: "非公開" - } - }, created_at: { label: "作成日時" }, diff --git a/packages/plugins/plugin-audit/src/translations/zh-CN.objects.generated.ts b/packages/plugins/plugin-audit/src/translations/zh-CN.objects.generated.ts index 48c0b35c94..135d38ca51 100644 --- a/packages/plugins/plugin-audit/src/translations/zh-CN.objects.generated.ts +++ b/packages/plugins/plugin-audit/src/translations/zh-CN.objects.generated.ts @@ -186,9 +186,6 @@ export const zhCNObjects: NonNullable = { label: "父评论", help: "可选的父评论，用于嵌套回复" }, - reply_count: { - label: "回复数" - }, author_id: { label: "作者" }, @@ -216,14 +213,6 @@ export const zhCNObjects: NonNullable = { edited_at: { label: "编辑时间" }, - visibility: { - label: "可见性", - options: { - public: "公开", - internal: "内部", - private: "私有" - } - }, created_at: { label: "创建时间" },