Skip to content

Commit 9bc403e

Browse files
os-samclaude
andauthored
docs(plugin-approvals): correct what the approval snapshot column is FOR (#11837)
`sys_approval_request`'s module docstring justified `payload_json` with "used by notifications so they can render before the record is locked or changed". That consumer does not exist. Measured against every `this.notify(...)` call site in `approval-service.ts` -- all 12 -- each passes `{ title, message, actionUrl }` (two also `actions`), built from `object_name` / `record_id` and the caller's comment. None reads `payload_json` or the parsed `payload`. The sentence was the only documented justification for the column holding a FULL row, and it was cited as such during the #10749 consumer inventory before anyone checked it. The docstring now states the real reason: the snapshot is retained as audit evidence of what was actually submitted (so the column stays whole at rest) and is served redacted per reader by the subject object's field-level read controls via `getReadableFields`, on the approvals-inbox door and the generic data door alike (#11039). The field's own `description` is deliberately unchanged -- it is accurate, and it, not the JSDoc, is the string extracted into the four generated i18n bundles. Proven rather than assumed: regenerating with the documented command left all four bundles byte-identical, and a positive control (mutating the extracted `description`) moved the `en` leaf, so the zero is a real zero. Also carries the residual documentation #10749's closure assigned to the next docs touch in this lane: `payload-redaction.ts` recorded hidden-vs-serialization as an OPEN `packages/spec` question, and it has since been ruled (maintainer, 2026-08-24). That paragraph now states the ruling -- `hidden: true` stays UI-only; `internal: true` is the serialization primitive -- and points an author who needs a field kept out of read results at `internal` (#7728, ADR-0049) rather than at `hidden`, which never governed serialization. Documentation only: no runtime behaviour, schema field, or type signature changes. The corrected JSDoc does ship to consumers in `dist/index.d.ts`, which is why this carries a patch changeset rather than none. Part of #11041 Claude-Session: https://claude.ai/code/session_01APWX2AwT3a4xDcjPCe8bk4 Co-authored-by: Claude <noreply@anthropic.com>
1 parent 78f65ef commit 9bc403e

3 files changed

Lines changed: 82 additions & 5 deletions

File tree

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
---
2+
"@objectstack/plugin-approvals": patch
3+
---
4+
5+
Correct what `sys_approval_request.payload_json` is documented to be FOR — it is audit evidence served redacted per reader, not a notification source
6+
7+
The object's module docstring — which ships to consumers in the package's type
8+
declarations — justified the snapshot column with: *"used by notifications so
9+
they can render before the record is locked or changed."* That consumer does
10+
not exist. Measured against every `this.notify(...)` call site in
11+
`approval-service.ts`, all **12** of them, each passes a payload of
12+
`{ title, message, actionUrl }` (two also carry `actions`), built from
13+
`object_name` / `record_id` and the caller's own comment. **None** reads
14+
`payload_json` or the parsed `payload`.
15+
16+
This is more than tidiness: that sentence was the only documented
17+
justification for the column holding a *full* row, and it was cited as such
18+
during the #10749 consumer inventory before anyone checked it. The docstring
19+
now states the real reason — the snapshot is retained as **audit evidence of
20+
what was actually submitted**, so the column stays whole at rest, and is served
21+
**redacted per reader** by the subject object's field-level read controls via
22+
`getReadableFields`, on the approvals-inbox door and the generic data door
23+
alike (#11039).
24+
25+
The field's own `description` is deliberately unchanged: `Record snapshot at
26+
submission time` is accurate, and it — unlike the JSDoc — is the string
27+
extracted into the four generated i18n bundles, so no translation leaf moves
28+
and no locale is left holding an English seed.
29+
30+
Also carried in the same pass, the residual documentation the #10749 closure
31+
assigned to the next docs touch in this lane: `payload-redaction.ts` recorded
32+
`hidden`-vs-serialization as an **open** `packages/spec` question, and it has
33+
since been ruled (maintainer, 2026-08-24, applying the 2026-08-12 lineage).
34+
That paragraph now states the ruling — **`hidden: true` stays UI-only;
35+
`internal: true` is the serialization primitive** — so an author who needs a
36+
field kept out of read results is pointed at `internal: true` (#7728,
37+
ADR-0049) rather than at `hidden`, which never governed serialization.
38+
39+
Documentation only: no runtime behaviour, no schema field, and no public type
40+
signature changes.

packages/plugins/plugin-approvals/src/payload-redaction.ts

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,22 @@
4444
* it. Making it govern serialization here alone would make the approval path
4545
* stricter than a direct read of the very same row (so it would close no
4646
* leak — the approver can just read the record), while breaking every drawer
47-
* that renders a `hidden` business column. That is a `packages/spec`
48-
* semantics question, and it is left open rather than decided here.
47+
* that renders a `hidden` business column.
48+
*
49+
* That `packages/spec` semantics question was left open here when this seam
50+
* landed. **It has since been ruled** (maintainer, 2026-08-24, applying the
51+
* 2026-08-12 lineage rather than making a new rule): **`hidden: true` stays
52+
* UI-only; `internal: true` is the serialization primitive.** `hidden`
53+
* gains no serialization semantic — it says "Hidden from default UI" and
54+
* nothing more. The read-side omission primitive is `internal: true`
55+
* (#7728, ADR-0049): the engine OMITS the key from `find`/`findOne` results,
56+
* the 201 create body and the by-id update body, on the default projection
57+
* AND when a client names the field in `?select=`. `internal` exists
58+
* PRECISELY BECAUSE `hidden` is not that, so an author who needs a field
59+
* kept out of read results declares `internal: true`; declaring `hidden`
60+
* and expecting omission is the mistake this paragraph exists to stop.
61+
* Per-caller field visibility — what this seam applies — remains
62+
* `requiredPermissions` / permission sets / `maskingRule`.
4963
*/
5064

5165
/** The slice of the security service this seam consumes. */

packages/plugins/plugin-approvals/src/sys-approval-request.object.ts

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,16 @@ import { APPROVAL_STATUSES, APPROVAL_STATUS_LABELS } from '@objectstack/spec/con
1717
* snapshots the Approval node config (approvers / behaviour) the request was
1818
* opened with.
1919
*
20-
* `payload_json` captures a snapshot of the target record at submission
21-
* time — used by notifications so they can render before the record is
22-
* locked or changed.
20+
* `payload_json` captures a snapshot of the target record at submission time.
21+
* It is retained as **audit evidence of what was actually submitted**, so the
22+
* column stays whole AT REST; it is served **redacted per reader** — the
23+
* SUBJECT object's field-level read controls are applied at serve time from
24+
* the security service's `getReadableFields`, on the approvals-inbox door and
25+
* on the generic data door alike (#11039).
26+
*
27+
* ⛔ Notifications are NOT a consumer of this snapshot, and the audit-evidence
28+
* sentence above — not "notifications need it" — is why the column holds a
29+
* full row. See the note on the field itself.
2330
*
2431
* @namespace sys
2532
*/
@@ -214,6 +221,22 @@ export const SysApprovalRequest = ObjectSchema.create({
214221
group: 'State',
215222
}),
216223

224+
// The module docstring above used to justify this column with "used by
225+
// notifications so they can render before the record is locked or
226+
// changed". That consumer does not exist, and the claim was cited as the
227+
// reason the column holds a FULL row before anyone checked it. Measured
228+
// against every `this.notify(...)` call site in `approval-service.ts` —
229+
// all 12 of them: each passes `{ title, message, actionUrl }` (two also
230+
// `actions`), built from `object_name` / `record_id` and the caller's
231+
// comment. None reads this column or the parsed `payload`. The real
232+
// readers are the serve path (`rowFromRequest` -> `payload`, redacted per
233+
// reader), the decide-time approver re-resolution and the org backfill,
234+
// both under SYSTEM_CTX, and the free-text predicate.
235+
//
236+
// The `description` below is deliberately UNCHANGED: it is accurate, and
237+
// it is extracted into the four generated i18n bundles as `help`. The
238+
// false sentence lived only in the JSDoc above, which is not extracted —
239+
// so this correction moves no translation leaf.
217240
payload_json: Field.textarea({
218241
label: 'Snapshot',
219242
required: false,

0 commit comments

Comments
 (0)