fix(approvals): correct the attachment read-shape rationale and share the id rule

Follow-up on this PR's own change, after ADR-0104 D3 wave 2 landed on main.

The fix in this PR is right; its stated cause was inverted. It says the
`sys_approval_action.attachments` column "stores rich descriptors — a fileId is
resolved to a full descriptor on write". The code says otherwise:
approval-service.ts writes `input.attachments` verbatim (a fileId string[]),
and there is no write-side descriptor resolution anywhere in the repo. What
actually happens is that the column stores an opaque sys_file id — the stored
form of every media field — and the ObjectQL read path expands it into
`{ id, name, size, mimeType, url }` on the way out. That resolver was already
in this PR's base commit, so the descriptors observed in listActions came from
the read path, not from storage.

Left as written, that inverted account would have been a false statement about
storage sitting in the protocol contract, in the changeset, and in a regression
test comment — and PR-5a has since made the stored form explicitly an id, so it
would also have been contradicted by the schema. Corrected in all three places.

Also names the three read forms the normalizer actually handles, rather than
one: the expanded value (normal), a bare id (nothing to expand it into), and a
legacy inline blob written before the cutover, whose keys are snake_case
(`file_id`, `mime_type`). The last of those had no test; it has one now. The
id-token test reuses `isFileIdToken` from @objectstack/spec/data — the
platform's single arbiter of "opaque id or URL?" — so this normalizer and the
engine's read resolver cannot drift apart on that question, and a non-id string
is now surfaced as a url rather than mislabelled an id.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SHpGw3GBA9aFpfwVArRWfd

Author: claude

.changeset/approval-attachment-descriptors.md

@@ -3,20 +3,27 @@
 "@objectstack/plugin-approvals": patch
 ---
 
-fix(approvals): return decision attachments as file descriptors, not "[object Object]" (#3504)
+fix(approvals): return decision attachments as file values, not "[object Object]" (#3504)
 
-The `sys_approval_action.attachments` file field stores rich descriptors
-(`{ id, name, url, mimeType, size }`), not bare fileId strings — a fileId passed
-to the decision is resolved to a full descriptor on write. But `rowFromAction`
-mapped the column with `.map(String)`, collapsing each descriptor object to the
+`sys_approval_action.attachments` is a `Field.file`, so the column **stores an
+opaque `sys_file` id** (ADR-0104 D3 — the stored form of every media field). The
+ObjectQL read path resolves that id into its expanded
+`{ id, name, size, mimeType, url }` form on the way out. But `rowFromAction`
+mapped the column with `.map(String)`, collapsing each expanded value to the
 literal string `"[object Object]"`. Every `listActions` consumer (the approval
 inbox timeline) then received garbage: the attachment chip had no filename and
 its id was `"[object Object]"`, so opening it 404'd.
 
 - `ApprovalActionRow.attachments` is now `ApprovalActionAttachment[]` — the
-  descriptor carries `id` + display `name` + a download `url`, so a consumer can
-  label and open an attachment without needing read access to the system
-  `sys_file` object (which regular approvers do not have). A bare-string fileId
-  still normalizes to `{ id }` for safety.
+  expanded file value plus its id, so a consumer can label and open an
+  attachment without needing read access to the system `sys_file` object (which
+  regular approvers do not have).
+- Three read forms are accepted: the expanded value (the normal case), a bare id
+  (nothing to expand it into — storage service absent, file not committed), and
+  a legacy inline blob written before file-as-reference (`file_id` /
+  `mime_type`), until the backfill converts it. The id test reuses the
+  platform's `isFileIdToken`, so this and the engine's read resolver cannot
+  disagree about what counts as an id.
 - The decision *input* (`ApprovalDecisionInput.attachments`) is unchanged — it
-  still takes fileId strings. Only the read shape changed.
+  still takes fileId strings, which is also exactly what the column stores. Only
+  the read shape changed.

packages/plugins/plugin-approvals/src/approval-service.test.ts

@@ -1959,17 +1959,18 @@ describe('ApprovalService — listActions attachments mapping (#3266)', () => {
     expect(approve?.attachments).toEqual([{ id: 'file_a' }]);
   });
 
-  // The real column value: `attachments` is a `Field.file` (multiple), which
-  // stores rich descriptors — NOT fileId strings. The old `.map(String)` turned
-  // each into "[object Object]", so the inbox chip had no name and 404'd on open.
-  // The mapping must pass the descriptor (id + name + url) through unmangled.
-  it('passes a stored file descriptor object through with its name and url', async () => {
+  // The normal case. The column STORES an opaque sys_file id (ADR-0104 D3);
+  // the ObjectQL read path resolves it into the expanded
+  // `{ id, name, size, mimeType, url }` form on the way out. The old
+  // `.map(String)` turned that object into "[object Object]", so the inbox chip
+  // had no name and 404'd on open. The mapping must pass it through unmangled.
+  it('passes the engine-expanded file value through with its name and url', async () => {
     const engine = makeFakeEngine();
     let n = 0;
     const svc = new ApprovalService({ engine: engine as any, clock: { now: () => new Date(1757000000000 + (n++) * 1000) } });
     const req = await svc.openNodeRequest(openInput(['u9']), CTX);
     await svc.decideNode(req.id, { decision: 'approve', actorId: 'u9' }, SYS);
-    // Simulate the Field.file column's real shape (what the engine returns).
+    // Simulate what the read path hands back after resolving the stored id.
     const row = engine._tables['sys_approval_action'].find((a: any) => a.action === 'approve');
     row.attachments = [
       { id: 'file_a', name: 'signed-contract.pdf', mimeType: 'application/pdf', size: 24, url: '/api/v1/storage/files/file_a' },
@@ -1980,4 +1981,24 @@ describe('ApprovalService — listActions attachments mapping (#3266)', () => {
       { id: 'file_a', name: 'signed-contract.pdf', mimeType: 'application/pdf', size: 24, url: '/api/v1/storage/files/file_a' },
     ]);
   });
+
+  // Rows written before file-as-reference hold an inline blob whose keys are
+  // snake_case (`file_id`, `mime_type`). They stay readable until the backfill
+  // converts them, so both casings must map — the same drift that made objectui
+  // stop recognising images when the expanded form arrived.
+  it('maps a legacy inline blob (file_id / mime_type) written before the cutover', async () => {
+    const engine = makeFakeEngine();
+    let n = 0;
+    const svc = new ApprovalService({ engine: engine as any, clock: { now: () => new Date(1757000000000 + (n++) * 1000) } });
+    const req = await svc.openNodeRequest(openInput(['u9']), CTX);
+    await svc.decideNode(req.id, { decision: 'approve', actorId: 'u9' }, SYS);
+    const row = engine._tables['sys_approval_action'].find((a: any) => a.action === 'approve');
+    row.attachments = [
+      { file_id: 'file_b', name: 'old.pdf', mime_type: 'application/pdf', size: 12, url: 'https://cdn/old.pdf' },
+    ];
+    const acts = await svc.listActions(req.id, SYS);
+    expect(acts.find(a => a.action === 'approve')?.attachments).toEqual([
+      { id: 'file_b', name: 'old.pdf', mimeType: 'application/pdf', size: 12, url: 'https://cdn/old.pdf' },
+    ]);
+  });
 });

packages/plugins/plugin-approvals/src/approval-service.ts

@@ -31,6 +31,7 @@ import type {
   ApprovalStatus,
   SharingExecutionContext,
 } from '@objectstack/spec/contracts';
+import { isFileIdToken } from '@objectstack/spec/data';
 import { isGrantActive } from '@objectstack/core';
 
 /**
@@ -259,22 +260,38 @@ function slaDueAt(createdAt: unknown, cfg: any): string | undefined {
 /**
  * Normalize one raw `attachments` entry into an {@link ApprovalActionAttachment}.
  *
- * The `sys_approval_action.attachments` file field stores **rich descriptors**
- * (`{ id, name, url, mimeType, size }`), not bare fileId strings — even when the
- * decision was recorded with a fileId, the field resolves it on write. The
- * original mapping did `String(entry)`, which turned each descriptor object into
- * the literal `"[object Object]"` — so the inbox timeline showed a nameless,
+ * `sys_approval_action.attachments` is a `Field.file` (multiple), so the column
+ * **stores opaque `sys_file` ids** — that is the stored form of every media
+ * field (ADR-0104 D3). What arrives here is whichever of three forms the read
+ * path produced:
+ *
+ *  1. the **expanded** `{ id, name, size, mimeType, url }` the ObjectQL read
+ *     path resolves a stored id into — the normal case;
+ *  2. a **bare id**, when there was nothing to expand it into (storage service
+ *     absent, file not committed);
+ *  3. a **legacy inline blob** (`{ file_id, name, mime_type, url, … }`) written
+ *     before file-as-reference, until the backfill converts it.
+ *
+ * The original mapping did `String(entry)`, which turned form 1 into the
+ * literal `"[object Object]"` — so the inbox timeline showed a nameless,
  * un-openable attachment chip (#3266 follow-up; caught by browser verification).
- * We pass the descriptor through, tolerating a bare-string fileId for safety.
+ *
+ * Note the casing: the expanded form carries `mimeType`, the legacy blob
+ * `mime_type`. Both are accepted for the duration of the migration window.
  */
 function normalizeActionAttachment(entry: any): ApprovalActionAttachment | undefined {
   if (entry == null) return undefined;
+  // Form 2 — a bare reference. `isFileIdToken` is the platform's single arbiter
+  // of "is this string an opaque file id, or a URL?", shared with the engine's
+  // read resolver, so the two cannot disagree about what counts as an id.
   if (typeof entry === 'string') {
     const id = entry.trim();
-    return id ? { id } : undefined;
+    if (!id) return undefined;
+    return isFileIdToken(id) ? { id } : { id, url: id };
   }
   if (typeof entry === 'object') {
-    const id = entry.id ?? entry.fileId ?? entry.file_id;
+    // Forms 1 and 3 — `file_id` is the legacy blob's key for the same thing.
+    const id = entry.id ?? entry.file_id;
     if (id == null || String(id) === '') return undefined;
     const mimeType = entry.mimeType ?? entry.mime_type;
     return {

packages/spec/src/contracts/approval-service.ts

@@ -200,11 +200,18 @@ export type ApprovalActionKind =
   | 'ooo_substitute';
 
 /**
- * A file attached to a decision action (#3266). Resolved from the
- * `sys_approval_action.attachments` file field, which stores rich descriptors
- * (not bare fileIds) — so the row carries the display name and a download URL
- * directly, and consumers never need read access to the system `sys_file`
- * object to label or open an attachment.
+ * A file attached to a decision action (#3266) — the READ shape of one
+ * `sys_approval_action.attachments` entry.
+ *
+ * The column **stores an opaque `sys_file` id** (ADR-0104 D3: that is the
+ * stored form of every media field). The name, size, MIME type and URL are not
+ * stored alongside it — the ObjectQL read path resolves the id into its
+ * expanded `FileValueSchema` form on the way out, and this interface is that
+ * form plus the id. So a consumer gets everything it needs to label and open an
+ * attachment without read access to the system `sys_file` object, while the
+ * write side stays a plain id (see `ApprovalDecisionInput.attachments`).
+ *
+ * Field names follow the expanded form — `mimeType`, not `mime_type`.
  */
 export interface ApprovalActionAttachment {
   /** The `sys_file` id — pass to `GET /storage/files/:id/url` for a signed URL. */