Skip to content

Commit 04b9776

Browse files
os-zhuangclaude
andauthored
feat(plugin-audit)!: retire sys_comment.visibility / reply_count (#4756, ADR-0049) (#4849)
Both fields were declared with zero runtime consumers — nothing in this repo, in objectui, or in cloud ever read or maintained either one. Maintainer decision on the enforce-or-remove question: remove both. Same disposition and the same stated reason as sys_attachment.share_type / .visibility in #2755 ("attachment access is derived from the parent record"). - visibility: a security-LOOKING key with no gate behind it — not consulted by enforceFeedsCapability, the #4630 record-level gates, the REST layer, or objectui's discussion panel, so a comment marked `private` was as visible as a `public` one (Prime Directive #10 in its textbook shape). No replacement key: after #4630, who can see a comment derives from the record thread_id names. An external/portal meaning for `internal` needs ADR-0090 D11 externalSharingModel first, and can return enforce-first. - reply_count: never incremented and readonly, so every row read 0 forever. Deliberately NOT replaced by an afterInsert/afterDelete roll-up — the predicate/bulk write-hook gaps tracked by #4770 / #4778 / #4779 are exactly where such a counter drifts, and a drifting counter is worse than none. Count parent_id children at read time instead. Existing DB columns stay as unmanaged leftovers, no migration (as in #2755). plugin-audit's four translation bundles regenerated by the documented extractor; a pin test fails loudly, prescription included, if either name is re-declared. Claude-Session: https://claude.ai/code/session_0176qgxgCXTJCUv4YFLtusP9 Co-authored-by: Claude <noreply@anthropic.com>
1 parent 799d098 commit 04b9776

8 files changed

Lines changed: 184 additions & 59 deletions
Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
---
2+
"@objectstack/plugin-audit": minor
3+
---
4+
5+
feat(plugin-audit)!: retire `sys_comment.visibility` and `sys_comment.reply_count` (#4756, ADR-0049)
6+
7+
Both fields were modelled with **zero** runtime consumers — nothing in this repo,
8+
in `objectui`, or in `cloud` ever read or maintained either one. ADR-0049
9+
enforce-or-remove; maintainer decision: remove both. Same disposition, and for
10+
the same stated reason, as `sys_attachment.share_type` / `sys_attachment.visibility`
11+
in #2755 ("attachment access is derived from the parent record").
12+
13+
**REMOVED — `sys_comment.visibility`** (`'public' | 'internal' | 'private'`,
14+
defaulted `'public'`).
15+
16+
This one is a **security-looking key with no gate behind it**, which is the
17+
primary reason it goes rather than stays. No code path consulted it: not
18+
`enforceFeedsCapability`, not the record-level gates added in #4630, not the
19+
REST layer, not objectui's discussion panel. A comment an author marked
20+
`private` was visible to exactly the same people as a `public` one — an app
21+
author (or an AI authoring metadata) reading the field list would reasonably
22+
believe otherwise, and get a silent security failure instead of an error. That
23+
is the Prime Directive #10 trap in its textbook shape.
24+
25+
There is **no replacement key**: after #4630, who can see a comment is decided
26+
by the record-level permissions of the record its `thread_id` names — one
27+
coherent rule. A per-row enum layered on top would be a second source of truth
28+
for the same question. The enum's only genuinely missing meaning ("hidden from
29+
external/portal principals") depends on external principals existing at all,
30+
which waits on ADR-0090 D11's `externalSharingModel`; today there is nobody to
31+
hide a comment from. This does not foreclose that design — when portals land,
32+
a visibility key can return **enforce-first**, with a real gate and tests.
33+
34+
**FROM → TO:** stop sending `visibility` on `sys_comment` writes; to restrict
35+
who sees a discussion, restrict who can read the record `thread_id` points at.
36+
37+
**REMOVED — `sys_comment.reply_count`** (`number`, `defaultValue: 0`,
38+
`readonly: true`).
39+
40+
Never incremented anywhere, and `readonly` meant an author could not set it by
41+
hand either, so every row read `0` forever — a UI binding an "N replies" badge
42+
to it rendered `0` for every thread. Deliberately **not** replaced by an
43+
`afterInsert`/`afterDelete` roll-up: the predicate/bulk write-hook gaps tracked
44+
by #4770 / #4778 / #4779 (a hook that returns early without a single-record id
45+
lets the whole bulk operation through) are exactly where a hook-maintained
46+
counter drifts — a bulk delete of replies would never decrement it. A counter
47+
that drifts is worse than no counter, because both the UI and an AI reading the
48+
record trust it. If a badge needs the number, aggregate `parent_id` children at
49+
read time; a designed roll-up can be revisited once #4775's family has settled
50+
bulk-hook semantics.
51+
52+
**FROM → TO:** replace reads of `reply_count` with a count of `sys_comment` rows
53+
whose `parent_id` is the comment's id.
54+
55+
**Stored data.** Existing databases keep both columns as **unmanaged leftovers**
56+
— no migration, matching #2755. What changes where:
57+
58+
- **Reads are loud everywhere.** The read-axis gates (#4134 / #4226) resolve
59+
field names from the object schema, not from the table, so a filter, sort,
60+
`select` or `expand` naming `visibility` / `reply_count` now answers
61+
`400 INVALID_FIELD` on every deployment, leftover column or not. A "0 replies"
62+
badge that silently lied becomes an error that names itself.
63+
- **Writes are loud on new databases only.** A database provisioned after this
64+
change has no such column, so the write fails at the driver and is mapped to
65+
the same `400 INVALID_FIELD` envelope. On a pre-existing database the leftover
66+
column still accepts a value nothing will ever read — record validation does
67+
not reject undeclared keys. Dropping the two columns is an optional manual
68+
cleanup, not a requirement.

docs/adr/0052-audit-is-not-the-activity-feed.md

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,16 @@ timeline. But weighing it against the implementation reality reversed that lean:
177177
| REST | ✅ generic data API | ❌ nested `/data/{obj}/{id}/feed` route unmounted (404) |
178178
| threads/mentions/reactions | ✅ fields already declared (`parent_id`, `reply_count`, `mentions`, `reactions`) | ✅ (but unreachable) |
179179

180+
> **Note (#4756).** The row above records the field list **as it stood at
181+
> decision time**, and the decision itself is unchanged. One of those fields has
182+
> since been retired: `sys_comment.reply_count` was declared but never
183+
> incremented by anything, so it read `0` on every row forever — removed under
184+
> ADR-0049 enforce-or-remove, with a reply count taken as an aggregate over
185+
> `parent_id` children at read time. Threading itself (`parent_id`), mentions and
186+
> reactions are unaffected. `sys_comment.visibility` went with it in the same
187+
> change (never consulted by any gate; comment visibility derives from the record
188+
> `thread_id` names, #4630).
189+
180190
Picking the durable, default, UI-wired system reaches "one backend" **now**, at
181191
near-zero risk. `service-feed`'s only real edge — one unified *typed* stream — is
182192
obtained on the chosen family by treating **`sys_activity` as the unified
Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
import { describe, it, expect } from 'vitest';
4+
import { SysComment } from './index.js';
5+
6+
/**
7+
* #4756 — `sys_comment.visibility` / `sys_comment.reply_count` are RETIRED
8+
* (ADR-0049 enforce-or-remove). Both were declared with zero runtime consumers
9+
* in this repo, in `objectui` and in `cloud`; the removal follows the
10+
* `sys_attachment.share_type` / `sys_attachment.visibility` precedent (#2755).
11+
*
12+
* This file exists so the removal cannot be undone by accident. Re-declaring
13+
* either field turns it red, and the failure message carries the prescription
14+
* — that is the whole point: an object field has no `retiredKey()` tombstone to
15+
* reject the name at authoring time the way a spec property does, so the pin
16+
* IS the tombstone for the platform-owned declaration.
17+
*
18+
* If a future change genuinely needs one of these names back, it arrives
19+
* enforce-first — with the gate/roll-up and its own tests — and updates this
20+
* file deliberately, never as collateral of an unrelated edit.
21+
*/
22+
23+
const RETIRED_FIELDS: ReadonlyArray<readonly [field: string, prescription: string]> = [
24+
[
25+
'visibility',
26+
'comment visibility derives from the record `thread_id` points at (#4630); '
27+
+ 'a per-row enum would be a second, unenforced source of truth. An external/'
28+
+ 'portal distinction must be designed against ADR-0090 D11 `externalSharingModel` first.',
29+
],
30+
[
31+
'reply_count',
32+
'count `parent_id` children at read time (#4756); a hook-maintained roll-up '
33+
+ 'drifts through the predicate/bulk write-hook gaps tracked by #4770 / #4778 / #4779.',
34+
],
35+
];
36+
37+
describe('sys_comment — retired fields stay retired (#4756)', () => {
38+
it.each(RETIRED_FIELDS)(
39+
'%s is not declared on sys_comment',
40+
(field, prescription) => {
41+
const fields = (SysComment as { fields?: Record<string, unknown> }).fields ?? {};
42+
expect(
43+
Object.keys(fields),
44+
`sys_comment.${field} was retired under ADR-0049 (#4756) — ${prescription}`,
45+
).not.toContain(field);
46+
},
47+
);
48+
49+
it.each(RETIRED_FIELDS)(
50+
'%s is not referenced by an index, highlight or title declaration either',
51+
(field) => {
52+
const object = SysComment as {
53+
indexes?: Array<{ fields?: string[] }>;
54+
highlightFields?: string[];
55+
titleFormat?: string;
56+
nameField?: string;
57+
displayNameField?: string;
58+
};
59+
const indexedFields = (object.indexes ?? []).flatMap((i) => i.fields ?? []);
60+
expect(indexedFields).not.toContain(field);
61+
expect(object.highlightFields ?? []).not.toContain(field);
62+
expect(object.titleFormat ?? '').not.toContain(field);
63+
expect([object.nameField, object.displayNameField]).not.toContain(field);
64+
},
65+
);
66+
67+
it('keeps the replacement paths both prescriptions point at', () => {
68+
const fields = (SysComment as { fields?: Record<string, unknown> }).fields ?? {};
69+
// `reply_count` → aggregate over children of `parent_id`.
70+
expect(Object.keys(fields)).toContain('parent_id');
71+
// `visibility` → the record-level permissions of what `thread_id` names.
72+
expect(Object.keys(fields)).toContain('thread_id');
73+
});
74+
});

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

Lines changed: 32 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,34 @@ import { ObjectSchema, Field } from '@objectstack/spec/data';
1515
* `sys_comment` when you want a focused threaded discussion surface
1616
* without the heavier Chatter envelope.
1717
*
18+
* ## Removed fields (ADR-0049 enforce-or-remove, #4756)
19+
*
20+
* Two fields were modelled here with **zero** runtime consumers — nothing in
21+
* this repo, in `objectui`, or in `cloud` ever read or maintained them. Under
22+
* ADR-0049 a declared-but-unenforced key is removed rather than left to lie,
23+
* following the `sys_attachment.share_type` / `sys_attachment.visibility`
24+
* precedent (#2755: "attachment access is derived from the parent record").
25+
*
26+
* - **`visibility`** (`'public' | 'internal' | 'private'`) — never consulted by
27+
* any gate: not `enforceFeedsCapability`, not the record-level gates added in
28+
* #4630, not the REST layer, not objectui's discussion panel. A comment marked
29+
* `private` was exactly as visible as a `public` one — a *security-looking*
30+
* lever with no gate behind it (Prime Directive #10). **Prescription:** comment
31+
* visibility is decided by the record-level permissions of the record
32+
* `thread_id` names (#4630) — there is no per-row override. A designed
33+
* external/portal distinction would have to be defined against ADR-0090 D11's
34+
* `externalSharingModel` first, and can return as an enforced key with tests.
35+
* - **`reply_count`** — never incremented; `readonly: true` meant an author
36+
* could not even set it by hand, so every row read `0` forever. **Prescription:**
37+
* count `parent_id` children at read time. Deliberately NOT re-introduced as a
38+
* hook-maintained roll-up: the predicate/bulk write-hook gaps tracked by
39+
* #4770 / #4778 / #4779 are exactly where such a counter drifts (a bulk delete
40+
* of replies would never decrement it), and a counter that drifts is worse
41+
* than no counter — both the UI and an AI reading the record would trust it.
42+
*
43+
* Existing databases keep the two columns as unmanaged leftovers; there is no
44+
* migration (same disposition as #2755).
45+
*
1846
* @namespace sys
1947
*/
2048
export const SysComment = ObjectSchema.create({
@@ -49,20 +77,15 @@ export const SysComment = ObjectSchema.create({
4977
group: 'Thread',
5078
}),
5179

80+
// The reply relationship is `parent_id` and nothing else — a reply count is
81+
// an aggregate over the children, computed at read time (#4756).
5282
parent_id: Field.lookup('sys_comment', {
5383
label: 'Parent Comment',
5484
required: false,
5585
description: 'Optional parent comment for nested replies',
5686
group: 'Thread',
5787
}),
5888

59-
reply_count: Field.number({
60-
label: 'Reply Count',
61-
defaultValue: 0,
62-
readonly: true,
63-
group: 'Thread',
64-
}),
65-
6689
// ── Author ───────────────────────────────────────────────────
6790
author_id: Field.lookup('sys_user', {
6891
label: 'Author',
@@ -119,14 +142,8 @@ export const SysComment = ObjectSchema.create({
119142
group: 'Lifecycle',
120143
}),
121144

122-
visibility: Field.select(
123-
['public', 'internal', 'private'],
124-
{
125-
label: 'Visibility',
126-
defaultValue: 'public',
127-
group: 'Lifecycle',
128-
},
129-
),
145+
// No `visibility` field: who can see a comment is decided by the record
146+
// `thread_id` points at (#4630, #4756) — one rule, no second source.
130147

131148
created_at: Field.datetime({
132149
label: 'Created At',

packages/plugins/plugin-audit/src/translations/en.objects.generated.ts

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -186,9 +186,6 @@ export const enObjects: NonNullable<TranslationData['objects']> = {
186186
label: "Parent Comment",
187187
help: "Optional parent comment for nested replies"
188188
},
189-
reply_count: {
190-
label: "Reply Count"
191-
},
192189
author_id: {
193190
label: "Author"
194191
},
@@ -216,14 +213,6 @@ export const enObjects: NonNullable<TranslationData['objects']> = {
216213
edited_at: {
217214
label: "Edited At"
218215
},
219-
visibility: {
220-
label: "Visibility",
221-
options: {
222-
public: "public",
223-
internal: "internal",
224-
private: "private"
225-
}
226-
},
227216
created_at: {
228217
label: "Created At"
229218
},

packages/plugins/plugin-audit/src/translations/es-ES.objects.generated.ts

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -186,9 +186,6 @@ export const esESObjects: NonNullable<TranslationData['objects']> = {
186186
label: "Comentario principal",
187187
help: "Comentario principal opcional para respuestas anidadas."
188188
},
189-
reply_count: {
190-
label: "Número de respuestas"
191-
},
192189
author_id: {
193190
label: "Autor"
194191
},
@@ -216,14 +213,6 @@ export const esESObjects: NonNullable<TranslationData['objects']> = {
216213
edited_at: {
217214
label: "Editado el"
218215
},
219-
visibility: {
220-
label: "Visibilidad",
221-
options: {
222-
public: "Público",
223-
internal: "Interno",
224-
private: "Privado"
225-
}
226-
},
227216
created_at: {
228217
label: "Creado el"
229218
},

packages/plugins/plugin-audit/src/translations/ja-JP.objects.generated.ts

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -186,9 +186,6 @@ export const jaJPObjects: NonNullable<TranslationData['objects']> = {
186186
label: "親コメント",
187187
help: "ネストした返信用のオプションの親コメント"
188188
},
189-
reply_count: {
190-
label: "返信数"
191-
},
192189
author_id: {
193190
label: "投稿者"
194191
},
@@ -216,14 +213,6 @@ export const jaJPObjects: NonNullable<TranslationData['objects']> = {
216213
edited_at: {
217214
label: "編集日時"
218215
},
219-
visibility: {
220-
label: "公開範囲",
221-
options: {
222-
public: "公開",
223-
internal: "内部",
224-
private: "非公開"
225-
}
226-
},
227216
created_at: {
228217
label: "作成日時"
229218
},

packages/plugins/plugin-audit/src/translations/zh-CN.objects.generated.ts

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -186,9 +186,6 @@ export const zhCNObjects: NonNullable<TranslationData['objects']> = {
186186
label: "父评论",
187187
help: "可选的父评论,用于嵌套回复"
188188
},
189-
reply_count: {
190-
label: "回复数"
191-
},
192189
author_id: {
193190
label: "作者"
194191
},
@@ -216,14 +213,6 @@ export const zhCNObjects: NonNullable<TranslationData['objects']> = {
216213
edited_at: {
217214
label: "编辑时间"
218215
},
219-
visibility: {
220-
label: "可见性",
221-
options: {
222-
public: "公开",
223-
internal: "内部",
224-
private: "私有"
225-
}
226-
},
227216
created_at: {
228217
label: "创建时间"
229218
},

0 commit comments

Comments
 (0)