fix(plugin-audit)!: an engine read of sys_activity returns only the rows whose parent record the caller can read

.changeset/20833-activity-parent-read-gate.md

@@ -0,0 +1,22 @@
+---
+'@objectstack/plugin-audit': minor
+---
+
+fix(plugin-audit)!: an engine read of `sys_activity` returns only the rows whose parent record the caller can read, the same way an engine read of `sys_comment` is narrowed
+
+Clause-②: no (narrowing)
+
+<!-- adr-0087: not-required (no-migration-prescription) a narrowing of what a READ returns, on one platform object, decided at request time: an activity row is served to a caller exactly when the record it names (object_name, record_id) is one that caller can read. No authorable key, spelling, value domain, export or stored metadata shape moves: the sys_activity object definition is byte-identical, every query shape parses as before, the package barrel exports nothing new and nothing less, and no stored row is rewritten. There is therefore nothing for an author to convert and nothing for `objectstack migrate meta` to reach. The other categories are closed on facts: the package publishes (not unpublished); no ADR-0087 id covers a read-visibility rule and this diff adds none (not registered / already-registered); and the change is runtime behaviour, not a published TypeScript declaration (not runtime-interface-only / type-surface-only). -->
+
+**BREAKING**: this narrows what an engine read of `sys_activity` returns to a caller that is not system context. It ships as `minor` under the repo's launch-window convention for narrowings.
+
+**What changes.** `AuditPlugin` now mounts a read gate on `sys_activity`, beside the one `sys_comment` already has. It is an engine middleware, so it narrows what passes through the engine: `find`, `findOne`, `count` and `aggregate`, which on the generic data doors are the list, its `total`, the by-id read and both query shapes. There a row is returned only when the caller can read the record it is about. That answer is the one the comment gate asks: the caller's own engine read of the parent record, so the parent object's sharing, RLS and object-level permissions decide. Parent reads are batched, one per parent object.
+
+**Rows that are left out**, failing closed exactly as the comment gate does for its threads:
+
+- a row about a record the caller cannot read;
+- a row about a record that no longer exists;
+- a row that names no record, or names an object the engine does not know;
+- a row that names `sys_activity` itself.
+
+**Unchanged.** A caller who can read every record (an admin) keeps every row about a record that exists. System-context reads, including the audit writer's own, are not narrowed. The object, its fields and what the CRUD mirror writes are unchanged, and nothing stored is rewritten.

content/docs/permissions/system-context.mdx

@@ -10,7 +10,7 @@ the seed loader replaying package fixtures, a plugin's boot reconciler, a
 service self-write, a migration.
 
 This page is **the authority** for what that flag actually does. It exists
-because the flag is not one concept: it is a single boolean read at **107
+because the flag is not one concept: it is a single boolean read at **108
 distinct sites across 19 packages**, and knowing three of those behaviours gives
 no hint that the other hundred-and-four exist. Every documented app-side bug
 traced to `isSystem` had the same shape — the metadata was complete and correct,
@@ -138,7 +138,7 @@ that silently does not happen.
 
 ### 3. Sharing (`plugin-sharing`)
 
-The largest single consumer — **17 of the 107 sites**.
+The largest single consumer — **17 of the 108 sites**.
 
 | # | Behaviour when `isSystem` | What you get / what you lose | Anchor |
 |:--|:---|:---|:---|
@@ -161,7 +161,7 @@ The largest single consumer — **17 of the 107 sites**.
 | 42 | Delegation write guard bypassed | plugin-approvals | Get: service / seed / import may write delegation rows naming another delegator | `packages/plugins/plugin-approvals/src/lifecycle-hooks.ts#bindDelegationWriteGuard` |
 | 43 | Approval actor / submitter / pending-approver checks bypassed (8 sites) | plugin-approvals | Get: approve, reject, recall, reassign without being a pending approver or the submitter | `packages/plugins/plugin-approvals/src/approval-service.ts#isOverrideActor`, `#resolveActor`, `#sendBack`, `#resubmit`, `#reassign`, `#remind`, `#requestInfo`, `#comment` |
 | 44 | Attachment access hooks return early (insert + update + delete, and the read AST) | service-storage | Lose: attachment visibility scoping | `packages/services/service-storage/src/attachment-access-hooks.ts#installAttachmentAccessHooks`, `#installAttachmentReadVisibility` |
-| 45 | Comment access hooks return early (insert + update + delete, and the read AST) | plugin-audit | Lose: comment visibility scoping | `packages/plugins/plugin-audit/src/comment-access-hooks.ts#installCommentAccessHooks`, `#installCommentReadVisibility` |
+| 45 | Comment access hooks return early (insert + update + delete, and the read AST), and so does the activity read gate (its read AST) | plugin-audit | Lose: comment visibility scoping, and the narrowing of `sys_activity` to rows whose parent record the caller can read | `packages/plugins/plugin-audit/src/comment-access-hooks.ts#installCommentAccessHooks`, `#installCommentReadVisibility`, `packages/plugins/plugin-audit/src/activity-read-visibility.ts#installActivityReadVisibility` |
 | 46 | Knowledge search returns hits unfiltered | service-knowledge | Lose: the permission filter over search results | `packages/services/service-knowledge/src/knowledge-service.ts#applyPermissionFilter` |
 
 ### 5. Actions, metadata plane, provenance, the organization wall
@@ -281,7 +281,7 @@ Ownership injection, `readonly` bypass and sharing materialisation are
 independent decisions, and a seed loader plausibly wants the first two but not
 the third. The concept is nevertheless **staying as one boolean**:
 
-- **Shipped semantics.** `isSystem` is a published contract with 107 read sites
+- **Shipped semantics.** `isSystem` is a published contract with 108 read sites
   in 19 packages. Splitting it is a breaking contract change across all of them.
   (The ruling was taken when the census read 80 sites in 18 packages; the count
   has grown, which strengthens rather than weakens the argument.)
@@ -355,16 +355,16 @@ still holds equal to the census on every pull request:
 | Appearances of the bare identifier `isSystem` in non-test sources | 813 | — |
 | — parsed as a declaration | 22 | ✅ |
 | — parsed as an object-literal / type key (producers and option objects) | 310 | — |
-| — parsed as a property **read** | 113 | ✅ |
+| — parsed as a property **read** | 114 | ✅ |
 | — parsed in some other syntactic position (a local, a cast, a conditional) | 9 | ✅ |
 | — the remainder: text inside comments and string literals | 358 | — |
 | Of those reads: reads of one of the unrelated metadata fields | 6 | ✅ |
-| Of those reads: reads of `ExecutionContext.isSystem` | **107** | ✅ |
-| — behaviour-bearing (rows 1–61 above) | 104 | ✅ |
+| Of those reads: reads of `ExecutionContext.isSystem` | **108** | ✅ |
+| — behaviour-bearing (rows 1–61 above) | 105 | ✅ |
 | — carry the flag onward only (rows 62–64 above) | 3 | ✅ |
 | Packages containing at least one elevation read | **19** | ✅ |
-| Files containing at least one elevation read | 45 | ✅ |
-| — the distinct symbols those reads live in — what this page anchors | 90 | ✅ |
+| Files containing at least one elevation read | 46 | ✅ |
+| — the distinct symbols those reads live in — what this page anchors | 91 | ✅ |
 | — of those files, the ones holding more than one read in one symbol | 8 | ✅ |
 
 The six rows marked — are a **dated decomposition, not a live claim**: they were
@@ -428,7 +428,7 @@ same resolver, and the same registration shape, that holds `docs/adr/**`.
 Renaming a symbol is now a loud red instead of a silent misdirection.
 
 ⚠️ **The precision that costs, priced here rather than buried.** A symbol anchor
-cannot say WHICH read inside a function it means, and **8** of the **45**
+cannot say WHICH read inside a function it means, and **8** of the **46**
 anchored files hold more than one read inside a single symbol. So the population
 check runs per file at symbol granularity: every file the census finds a read in
 must be anchored, and the set of symbols this page cites into that file must

packages/plugins/plugin-audit/src/activity-read-visibility.integration.test.ts

@@ -0,0 +1,233 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * `sys_activity` read visibility, on the plugin's READ PATH.
+ *
+ * A caller holding object-level `sys_activity` read gets back only the
+ * activity rows whose parent record (`object_name`, `record_id`) it can read,
+ * the same way `installCommentReadVisibility` narrows `sys_comment`. An admin
+ * is the caller who can read every record, so it keeps every row about a
+ * record that exists.
+ *
+ * ## Why a real engine, a real driver and the real plugin
+ *
+ * The narrowing is a WHERE the gate ANDs into the caller's query. Whether that
+ * WHERE selects the right rows is a question about the Filter Protocol as a
+ * driver executes it (`$or` over two columns, `$in`, `count`, `aggregate`), not
+ * about the shape of an object, so the rows below are written by the real
+ * audit writer (the CRUD mirror is the platform's own producer of these rows),
+ * stored by a real SQLite driver and read back through the real engine
+ * middleware chain. The gate is mounted by `AuditPlugin` itself at
+ * `kernel:ready`, so a gate that stops being mounted fails here exactly like a
+ * gate that was never written.
+ *
+ * ## The one stand-in, and why it is the smallest honest one
+ *
+ * PARENT READABILITY. In a deployment the parent object's own sharing, RLS and
+ * object-level CRUD decide whether a caller can read a record; the gate only
+ * ever asks the engine, under the caller's context. Here one engine middleware
+ * on the owned parent object plays that part: a member reads the ledger rows
+ * it owns, the admin reads them all. The gate cannot tell it apart from
+ * plugin-security because it asks the same question the same way.
+ */
+
+import { describe, it, expect, beforeAll, afterAll } from 'vitest';
+import { ObjectKernel } from '@objectstack/core';
+import { ObjectQL, ObjectQLPlugin } from '@objectstack/objectql';
+import { SqliteWasmDriver } from '@objectstack/driver-sqlite-wasm';
+
+import { AuditPlugin } from './audit-plugin.js';
+
+const ACTIVITY = 'sys_activity';
+/** A parent object whose rows each caller reads only when it owns them. */
+const OWNED = 'act_ledger';
+/** A parent object every caller reads. */
+const OPEN = 'act_board';
+const HARNESS_PACKAGE = 'com.objectstack.audit.test.activity-read';
+
+const SYS = { isSystem: true } as const;
+const MEMBER = { userId: 'u_member', tenantId: 'org_1', positions: ['org_member'] };
+const OTHER = { userId: 'u_other', tenantId: 'org_1', positions: ['org_member'] };
+const ADMIN = { userId: 'u_admin', tenantId: 'org_1', positions: ['org_admin'] };
+
+const ownedObject = {
+  name: OWNED,
+  label: 'Ledger',
+  fields: {
+    name: { name: 'name', label: 'Name', type: 'text' as const },
+    owner: { name: 'owner', label: 'Owner', type: 'text' as const },
+  },
+};
+const openObject = {
+  name: OPEN,
+  label: 'Board',
+  fields: { name: { name: 'name', label: 'Name', type: 'text' as const } },
+};
+
+const READ_OPS = new Set(['find', 'findOne', 'count', 'aggregate']);
+
+/** Parent-object read operations issued under a caller (non-system) context. */
+const parentProbes: Array<{ object: string; operation: string; userId?: string }> = [];
+
+type Row = Record<string, any>;
+const pair = (r: Row) => `${r.object_name ?? 'NULL'}/${r.record_id ?? 'NULL'}`;
+const pairs = (rows: Row[]) => rows.map(pair).sort();
+
+describe('sys_activity read visibility — the plugin read path', () => {
+  let kernel: ObjectKernel;
+  let engine: ObjectQL;
+  const ids: Record<string, string> = {};
+  let allRows: Row[] = [];
+
+  beforeAll(async () => {
+    kernel = new ObjectKernel({ logger: { level: 'silent' } });
+    await kernel.use(new ObjectQLPlugin());
+    await kernel.use(new AuditPlugin());
+    await kernel.bootstrap();
+
+    engine = kernel.getService<ObjectQL>('objectql');
+    const driver = new SqliteWasmDriver({ filename: ':memory:' });
+    await driver.connect();
+    engine.registerDriver(driver, true);
+    engine.registry.registerObject(ownedObject as any, HARNESS_PACKAGE);
+    engine.registry.registerObject(openObject as any, HARNESS_PACKAGE);
+    await engine.syncSchemas();
+
+    // Parent readability: a non-admin caller reads the ledger rows it owns.
+    engine.registerMiddleware(async (op: any, next: () => Promise<void>) => {
+      if (READ_OPS.has(op.operation) && op.context && !op.context.isSystem) {
+        parentProbes.push({ object: op.object, operation: op.operation, userId: op.context.userId });
+        if (op.context.userId !== ADMIN.userId && op.ast) {
+          const mine = { owner: op.context.userId ?? '__nobody__' };
+          op.ast.where = op.ast.where ? { $and: [op.ast.where, mine] } : mine;
+        }
+      }
+      return next();
+    }, { object: OWNED });
+    engine.registerMiddleware(async (op: any, next: () => Promise<void>) => {
+      if (READ_OPS.has(op.operation) && op.context && !op.context.isSystem) {
+        parentProbes.push({ object: op.object, operation: op.operation, userId: op.context.userId });
+      }
+      return next();
+    }, { object: OPEN });
+
+    // Every row below is written by the real audit writer's CRUD mirror.
+    ids.mine = (await engine.insert(OWNED, { name: 'mine', owner: MEMBER.userId }, { context: SYS })).id;
+    ids.theirs = (await engine.insert(OWNED, { name: 'theirs', owner: OTHER.userId }, { context: SYS })).id;
+    await engine.update(OWNED, { name: 'theirs, renamed' }, { where: { id: ids.theirs }, context: SYS });
+    ids.board1 = (await engine.insert(OPEN, { name: 'board one' }, { context: SYS })).id;
+    ids.board2 = (await engine.insert(OPEN, { name: 'board two' }, { context: SYS })).id;
+    ids.gone = (await engine.insert(OPEN, { name: 'board gone' }, { context: SYS })).id;
+    await engine.delete(OPEN, { where: { id: ids.gone }, context: SYS });
+
+    // Rows no CRUD mirror writes, but an app's own server action can: one that
+    // names no parent, one that names an object no registry knows, and one
+    // that names the activity stream itself.
+    const stamp = new Date().toISOString();
+    for (const row of [
+      { type: 'note', summary: 'no parent', object_name: null, record_id: null },
+      { type: 'note', summary: 'unknown parent', object_name: 'act_unregistered', record_id: 'x1' },
+      { type: 'note', summary: 'self parent', object_name: ACTIVITY, record_id: 'a1' },
+    ]) {
+      await engine.insert(ACTIVITY, { ...row, timestamp: stamp }, { context: SYS });
+    }
+
+    allRows = await engine.find(ACTIVITY, { context: SYS });
+  }, 120_000);
+
+  afterAll(async () => {
+    if (kernel) {
+      await Promise.race([kernel.shutdown(), new Promise<void>((r) => setTimeout(r, 10_000))]);
+    }
+  }, 30_000);
+
+  it('control: the CRUD mirror and the direct writes produced every row this file reasons about', () => {
+    expect(pairs(allRows)).toEqual(
+      [
+        `${OPEN}/${ids.board1}`,
+        `${OPEN}/${ids.board2}`,
+        `${OPEN}/${ids.gone}`,
+        `${OPEN}/${ids.gone}`,
+        `${OWNED}/${ids.mine}`,
+        `${OWNED}/${ids.theirs}`,
+        `${OWNED}/${ids.theirs}`,
+        `${ACTIVITY}/a1`,
+        'NULL/NULL',
+        'act_unregistered/x1',
+      ].sort(),
+    );
+  });
+
+  it('control: the parent stand-in answers a member exactly the records it owns', async () => {
+    const owned = await engine.find(OWNED, { context: MEMBER });
+    expect(owned.map((r: Row) => r.id)).toEqual([ids.mine]);
+  });
+
+  it('find: a member reads only the activity of records it can read', async () => {
+    const rows = await engine.find(ACTIVITY, { context: MEMBER });
+    expect(pairs(rows)).toEqual([`${OPEN}/${ids.board1}`, `${OPEN}/${ids.board2}`, `${OWNED}/${ids.mine}`].sort());
+  });
+
+  it('find: another member reads its own record’s activity, not the first member’s', async () => {
+    const rows = await engine.find(ACTIVITY, { context: OTHER });
+    expect(pairs(rows)).toEqual(
+      [`${OPEN}/${ids.board1}`, `${OPEN}/${ids.board2}`, `${OWNED}/${ids.theirs}`, `${OWNED}/${ids.theirs}`].sort(),
+    );
+  });
+
+  it('count: the list total is narrowed identically to the rows', async () => {
+    expect(await engine.count(ACTIVITY, {}, { context: MEMBER })).toBe(3);
+  });
+
+  it('findOne: a row whose parent the member cannot read is absent by id', async () => {
+    const hidden = allRows.find((r) => r.object_name === OWNED && r.record_id === ids.theirs)!;
+    const shown = allRows.find((r) => r.object_name === OWNED && r.record_id === ids.mine)!;
+    expect(await engine.findOne(ACTIVITY, { where: { id: hidden.id }, context: MEMBER })).toBeNull();
+    expect((await engine.findOne(ACTIVITY, { where: { id: shown.id }, context: MEMBER }))?.id).toBe(shown.id);
+  });
+
+  it('aggregate: a grouped count sees only the readable rows', async () => {
+    const groups = await engine.aggregate(
+      ACTIVITY,
+      { groupBy: ['object_name'], aggregations: [{ function: 'count', alias: 'n' }], context: MEMBER },
+    );
+    const byObject = Object.fromEntries(groups.map((g: Row) => [g.object_name ?? 'NULL', Number(g.n)]));
+    expect(byObject).toEqual({ [OPEN]: 2, [OWNED]: 1 });
+  });
+
+  it('a query scoped to a record the member cannot read returns nothing', async () => {
+    const rows = await engine.find(
+      ACTIVITY,
+      { where: { object_name: OWNED, record_id: ids.theirs }, context: MEMBER },
+    );
+    expect(rows).toEqual([]);
+  });
+
+  it('rows whose parent no longer exists, or that name no readable parent, stay out', async () => {
+    const rows = await engine.find(ACTIVITY, { context: MEMBER });
+    const seen = new Set(rows.map((r: Row) => r.object_name ?? 'NULL'));
+    expect(rows.some((r: Row) => r.record_id === ids.gone)).toBe(false);
+    for (const name of ['NULL', 'act_unregistered', ACTIVITY]) expect(seen.has(name)).toBe(false);
+  });
+
+  it('a list read probes each parent object once, not once per row', async () => {
+    parentProbes.length = 0;
+    await engine.find(ACTIVITY, { context: OTHER });
+    const perObject = parentProbes.filter((p) => p.userId === OTHER.userId).map((p) => `${p.object}:${p.operation}`);
+    expect(perObject.sort()).toEqual([`${OPEN}:find`, `${OWNED}:find`]);
+  });
+
+  it('an admin, who reads every record, keeps every row about a record that exists', async () => {
+    const rows = await engine.find(ACTIVITY, { context: ADMIN });
+    const existing = allRows.filter(
+      (r) => (r.object_name === OWNED || r.object_name === OPEN) && r.record_id !== ids.gone,
+    );
+    expect(pairs(rows)).toEqual(pairs(existing));
+    expect(await engine.count(ACTIVITY, {}, { context: ADMIN })).toBe(existing.length);
+  });
+
+  it('a system read is not narrowed', async () => {
+    const rows = await engine.find(ACTIVITY, { context: SYS });
+    expect(rows.length).toBe(allRows.length);
+  });
+});