Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 68 additions & 0 deletions .changeset/sys-comment-retire-visibility-reply-count.md
Original file line number Diff line number Diff line change
@@ -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.
10 changes: 10 additions & 0 deletions docs/adr/0052-audit-is-not-the-activity-feed.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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<readonly [field: string, prescription: string]> = [
[
'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<string, unknown> }).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<string, unknown> }).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');
});
});
47 changes: 32 additions & 15 deletions packages/plugins/plugin-audit/src/objects/sys-comment.object.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand Down Expand Up @@ -49,20 +77,15 @@ 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,
description: 'Optional parent comment for nested replies',
group: 'Thread',
}),

reply_count: Field.number({
label: 'Reply Count',
defaultValue: 0,
readonly: true,
group: 'Thread',
}),

// ── Author ───────────────────────────────────────────────────
author_id: Field.lookup('sys_user', {
label: 'Author',
Expand Down Expand Up @@ -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',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,9 +186,6 @@ export const enObjects: NonNullable<TranslationData['objects']> = {
label: "Parent Comment",
help: "Optional parent comment for nested replies"
},
reply_count: {
label: "Reply Count"
},
author_id: {
label: "Author"
},
Expand Down Expand Up @@ -216,14 +213,6 @@ export const enObjects: NonNullable<TranslationData['objects']> = {
edited_at: {
label: "Edited At"
},
visibility: {
label: "Visibility",
options: {
public: "public",
internal: "internal",
private: "private"
}
},
created_at: {
label: "Created At"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,9 +186,6 @@ export const esESObjects: NonNullable<TranslationData['objects']> = {
label: "Comentario principal",
help: "Comentario principal opcional para respuestas anidadas."
},
reply_count: {
label: "Número de respuestas"
},
author_id: {
label: "Autor"
},
Expand Down Expand Up @@ -216,14 +213,6 @@ export const esESObjects: NonNullable<TranslationData['objects']> = {
edited_at: {
label: "Editado el"
},
visibility: {
label: "Visibilidad",
options: {
public: "Público",
internal: "Interno",
private: "Privado"
}
},
created_at: {
label: "Creado el"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,9 +186,6 @@ export const jaJPObjects: NonNullable<TranslationData['objects']> = {
label: "親コメント",
help: "ネストした返信用のオプションの親コメント"
},
reply_count: {
label: "返信数"
},
author_id: {
label: "投稿者"
},
Expand Down Expand Up @@ -216,14 +213,6 @@ export const jaJPObjects: NonNullable<TranslationData['objects']> = {
edited_at: {
label: "編集日時"
},
visibility: {
label: "公開範囲",
options: {
public: "公開",
internal: "内部",
private: "非公開"
}
},
created_at: {
label: "作成日時"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,9 +186,6 @@ export const zhCNObjects: NonNullable<TranslationData['objects']> = {
label: "父评论",
help: "可选的父评论,用于嵌套回复"
},
reply_count: {
label: "回复数"
},
author_id: {
label: "作者"
},
Expand Down Expand Up @@ -216,14 +213,6 @@ export const zhCNObjects: NonNullable<TranslationData['objects']> = {
edited_at: {
label: "编辑时间"
},
visibility: {
label: "可见性",
options: {
public: "公开",
internal: "内部",
private: "私有"
}
},
created_at: {
label: "创建时间"
},
Expand Down
Loading