fix(audit-trail): close the remaining scope oracles and parity gaps with Ruby [PRD-1269]

packages/agent/package.json

@@ -31,6 +31,8 @@
     "light-my-request": "^5.14.0",
     "luxon": "^3.2.1",
     "object-hash": "^3.0.0",
+    "pg": "^8.8.0",
+    "pg-hstore": "^2.3.4",
     "sequelize": "^6.37.8",
     "superagent": "^10.3.0",
     "umzug": "^3.8.2",

packages/agent/src/audit-trail/README.md

@@ -16,8 +16,8 @@ The audit trail has two decoupled parts, both handled by the agent:
 Pass an `auditTrail` option to `createAgent`. The agent constructs the SQL store, registers the
 CRUD hooks on every collection, and mounts the record-history routes only when this option is set.
 
-Sequelize does not bundle database drivers, so install the one matching `connectionString`'s
-dialect — e.g. `pg` and `pg-hstore` for PostgreSQL — even in an agent whose main datasource isn't SQL.
+The agent ships `sequelize`, `umzug` and the PostgreSQL driver (`pg`, `pg-hstore`), so the option is all
+it takes, even in an agent whose main datasource isn't SQL.
 
 ```ts
 import { createAgent } from '@forestadmin/agent';
@@ -211,6 +211,22 @@ audit read; a record deleted in between is treated as a request starting a momen
 treated it, and one moved out of the caller's scope in between is refused. The extra read only
 happens for a scoped caller — with no scope there is nothing to withhold.
 
+Both reads count, and neither can clear the other, as in agent-ruby. A record gone at either read is
+treated as gone: an id freed by a delete and taken by another record since answers for itself, never
+for the rows of the record that held it before. And the re-read runs even for a record already gone
+at the first check, so an id since taken by a record the caller cannot read is refused (404).
+
+**An id freed and taken since has two lives.** A live record in the caller's scope says nothing
+about the rows filed under its id before it took the id: they are an earlier record's. The boundary
+is the id's latest confirmed `delete` (the earlier record's last row) or `create` (the current
+record's first), whichever is later; the `create` covers an id freed with no confirmed `delete`, by a
+primary-key move or a delete that was never confirmed. Those rows go through the same withholding as
+a record gone for good — on the history route (with `search` and `fields` matched against the served
+values, so the count and the authors cannot leak them either), on the correlation lookups, and on
+`/state`, whose reconstruction at an instant before the boundary is tested against the scope
+(strictly before: at the boundary's own instant the state already includes a replacement `create`
+sharing it). A `pending` row marks no boundary, since it may never have landed.
+
 That test only runs when the snapshot can actually answer it. The capture keeps the writable columns
 (plus the packed record id), so a scope reaching for anything else — a read-only column, a relation,
 a field stored redacted — has no honest answer in the snapshot and the values are withheld rather
@@ -310,11 +326,12 @@ the second read of the record decides the withholding, so the SQL-matched answer
 the history scanned the same way.
 
 Matching the *serialized* text rather than a structural walk of the parsed value is cheap and still
-correct for "keys and scalar values" — but two things follow from it. A punctuation-only term (`,`,
-`:`, `{`) matches almost any row whose diff has more than one key, since those characters are JSON
-structure rather than content. And a value containing a double quote or a backslash can't be found
-by searching for it literally — `5"` is stored in the JSON text as `5\"`, so searching `5"` never
-matches the row that holds it. Neither is severe, and both are inherent to the approach.
+correct for "keys and scalar values". The term is escaped the way JSON escaped the values, so `5"`
+finds the row whose JSON text holds `5\"`, and a bare quote can't match the document's own
+structure. The redaction mask is removed before matching, so `search=redacted` confirms nothing.
+Both hold on the served-value path too, and both mirror agent-ruby. One thing remains inherent to
+the approach: a punctuation-only term (`,`, `:`, `{`) matches almost any row whose diff has more
+than one key, since those characters are JSON structure rather than content.
 
 Defensive parsing:
 
@@ -351,20 +368,23 @@ The cursor follows the direction — `before`, an inclusive upper bound, newest
 inclusive lower bound, oldest first — and `meta.cursor` hands back the one the walk uses. The bound
 for the other direction is ignored.
 
-**Detail values follow the same rule as the per-record routes:** a caller who can read a collection
-gets its rows with `previousValues` and `newValues`, whatever their permission level. Someone who can
+**Detail values follow the same rule as the per-record routes:** a caller who can read a collection,
+with no scope on it, gets its rows with `previousValues` and `newValues`, whatever their permission
+level. Someone who can
 read the collection can already read each record's history one at a time, so the feed serves the same
 values in aggregate. Who can open the project's Activity Logs at all is decided per team by Forest
 (the `activityLogs` feature a team can be deactivated for), not by this route. The capabilities
 payload advertises the route as `canUseAuditTrailTimeline`, true only when the store implements
 `listTimeline`, and `canSortAuditTrailTimeline` alongside it: an agent without that flag serves the
 feed newest first whatever `sort` asks. A custom `listTimeline` must honour `order` and `after`.
 
-**Permissions.** Only the collections the caller can read are queried. A collection the caller sees
-only through a **record-level scope is left out entirely**: a scope can't be evaluated across a whole
-timeline without fetching every record it mentions, and the row alone would already reveal that a
-record the caller cannot read exists and was touched. A caller scoped on every collection therefore
-gets an empty timeline.
+**Permissions.** Only the collections the caller can read are queried. A collection the caller's
+team has a **record-level scope on is left out entirely**, whatever the scope's condition, even one
+every record passes: a scope can't be evaluated across a whole timeline without fetching every record
+it mentions. A caller scoped on every collection therefore gets an empty timeline. On the project's
+Activity Logs page, such a collection still shows its Forest activity logs, without diffs, since
+Forest never filtered those by roles or scopes; each record's own history route still serves its
+audit trail.
 
 **Filters** are the per-record route's, minus `fields` (a column name means nothing across
 collections): `userIds`, `operations`, `startDate` / `endDate` / `timezone`, `search`, parsed exactly

packages/agent/src/audit-trail/earlier-life.ts

@@ -0,0 +1,77 @@
+import type { AuditRecord, AuditStore } from './types';
+
+// A delete frees the id, and a later record can take it. A live record under the caller's scope says
+// nothing about the rows filed under that id before it took the id: they are another record's, and
+// its values go through the same withholding as a record gone for good.
+//
+// A confirmed `create` marks the boundary too: a primary-key move or an unconfirmed delete frees an
+// id without leaving a confirmed `delete`, and the record that takes it next starts with its create.
+
+/**
+ * Whether a row belongs to a record that held the id before the boundary, in the trail's
+ * (timestamp, id) order. A boundary `delete` is the earlier record's last row, a boundary `create`
+ * the current record's first.
+ */
+export function belongsToEarlierLife(row: AuditRecord, boundary: AuditRecord | null): boolean {
+  if (!boundary) return false;
+  if (row.id === boundary.id) return boundary.operation === 'delete';
+
+  return (
+    row.timestamp < boundary.timestamp ||
+    (row.timestamp === boundary.timestamp && row.id < boundary.id)
+  );
+}
+
+/**
+ * The id's latest confirmed `delete` or `create`, or null when the id has had one life only: no
+ * boundary at all, or a `create` with nothing filed before it.
+ */
+export async function lifeBoundaryOf(
+  store: AuditStore,
+  collection: string,
+  recordId: string,
+): Promise<AuditRecord | null> {
+  const boundaries = await store.listByRecord({
+    collection,
+    recordId,
+    operations: ['delete', 'create'],
+    order: 'desc',
+  });
+
+  // A pending row may never have landed, so it marks nothing. Filtered here too, not just in the
+  // query: a store that ignores `operations` must not turn any row into the boundary.
+  const boundary =
+    boundaries.find(
+      row => (row.operation === 'delete' || row.operation === 'create') && row.status !== 'pending',
+    ) ?? null;
+
+  if (boundary?.operation !== 'create') return boundary;
+
+  // A create with nothing filed before it began the id's only life. A record created through Forest
+  // has its own create here, and counting it would send every scoped search on it down the
+  // served-value scan. Tested on whatever comes back, should a store ignore the order or the limit.
+  const oldest = await store.listByRecord({ collection, recordId, order: 'asc', limit: 1 });
+
+  return oldest.some(row => belongsToEarlierLife(row, boundary)) ? boundary : null;
+}
+
+/** The rows as served: all withheld for a gone record, only the earlier lives' for a live one. */
+export function withholdEarlierLives(
+  rows: AuditRecord[],
+  {
+    gone,
+    boundary,
+    withhold,
+  }: {
+    gone: boolean;
+    boundary: AuditRecord | null;
+    withhold: (rows: AuditRecord[]) => AuditRecord[];
+  },
+): AuditRecord[] {
+  if (gone) return withhold(rows);
+  if (!boundary) return rows;
+
+  const withheld = withhold(rows);
+
+  return rows.map((row, index) => (belongsToEarlierLife(row, boundary) ? withheld[index] : row));
+}

packages/agent/src/audit-trail/index.ts

@@ -10,13 +10,17 @@ export {
   createSqlAuditStore,
   ensureAuditStorage,
   defineAuditLogModel,
-  fieldsChangedCondition,
-  searchCondition,
   toRow,
   fromRow,
   DEFAULT_SCHEMA,
   DEFAULT_TABLE,
 } from './sql-store';
 export { runAuditMigrations } from './migrations';
+export {
+  fieldsChangedCondition,
+  jsonEscaped,
+  matchesServedValues,
+  searchCondition,
+} from './value-filters';
 export { default as revertRecord } from './revert';
 export * from './types';

packages/agent/src/audit-trail/migrations.ts

@@ -1,4 +1,4 @@
-import type { QueryInterface, Sequelize, Transaction } from 'sequelize';
+import type { QueryInterface, Sequelize, Transaction, WhereOptions } from 'sequelize';
 
 import { DataTypes } from 'sequelize';
 import { SequelizeStorage, Umzug } from 'umzug';
@@ -35,6 +35,14 @@ function qualifiedMigrationName002(schema: string | undefined, tableName: string
     : `${tableName}:002-index-timestamp-id`;
 }
 
+function qualifiedMigrationName003(schema: string | undefined, tableName: string): string {
+  return schema
+    ? `${schema}.${tableName}:003-index-record-history`
+    : `${tableName}:003-index-record-history`;
+}
+
+const DIALECTS_WITHOUT_PARTIAL_INDEXES = new Set(['mysql', 'mariadb']);
+
 // Every table name claimed by an audit-trail store configured in this process, keyed by
 // `schema\0name` — both its own data table and the migration table Umzug derives from it.
 // A second store's data table can otherwise land on the exact name a first store's migration
@@ -74,6 +82,31 @@ async function indexNames(
   return new Set(indexes.map(index => index.name));
 }
 
+async function addIndexOnce(
+  context: MigrationContext,
+  name: string,
+  options: { fields: Array<string | { name: string; length: number }>; where?: WhereOptions },
+): Promise<void> {
+  const table = { tableName: context.tableName, schema: context.schema };
+  const hasIndex = () =>
+    indexNames(context.queryInterface, table, context.transaction).then(names => names.has(name));
+
+  // Idempotent for the same reason 001 is: a process losing a concurrent-boot race retries.
+  if (await hasIndex()) return;
+
+  try {
+    await context.queryInterface.addIndex(table, {
+      ...options,
+      name,
+      transaction: context.transaction,
+    });
+  } catch (error) {
+    // Without Postgres's advisory lock, another agent booting at the same moment can add the
+    // index between the check above and this statement. Anything else is a real failure.
+    if (!(await hasIndex().catch(() => false))) throw error;
+  }
+}
+
 async function columnNames(
   queryInterface: QueryInterface,
   table: { tableName: string; schema?: string },
@@ -210,27 +243,9 @@ function buildMigrations(schema: string | undefined, tableName: string) {
       // record, ordered and paged by (timestamp, id): without this, each page sorts the whole table.
       name: qualifiedMigrationName002(schema, tableName),
       up: async ({ context }: { context: MigrationContext }) => {
-        const table = { tableName: context.tableName, schema: context.schema };
-        const name = `${context.tableName}_timestamp_id`;
-        const hasIndex = () =>
-          indexNames(context.queryInterface, table, context.transaction).then(names =>
-            names.has(name),
-          );
-
-        // Idempotent for the same reason 001 is: a process losing a concurrent-boot race retries.
-        if (await hasIndex()) return;
-
-        try {
-          await context.queryInterface.addIndex(table, {
-            fields: ['timestamp', 'id'],
-            name,
-            transaction: context.transaction,
-          });
-        } catch (error) {
-          // Without Postgres's advisory lock, another agent booting at the same moment can add the
-          // index between the check above and this statement. Anything else is a real failure.
-          if (!(await hasIndex().catch(() => false))) throw error;
-        }
+        await addIndexOnce(context, `${context.tableName}_timestamp_id`, {
+          fields: ['timestamp', 'id'],
+        });
       },
       down: async ({ context }: { context: MigrationContext }) => {
         await context.queryInterface.removeIndex(
@@ -240,6 +255,45 @@ function buildMigrations(schema: string | undefined, tableName: string) {
         );
       },
     },
+    {
+      // A record's history is read by (collection, record_id) in (timestamp, id) order. With only
+      // `record_id` and `(timestamp, id)` to choose from, Postgres either reads every row the id
+      // holds across all collections, or walks the whole table's (timestamp, id) index under a
+      // LIMIT: finding a recent record's oldest row discarded 2.1M rows (534 ms) on a 2.1M-row table.
+      name: qualifiedMigrationName003(schema, tableName),
+      up: async ({ context }: { context: MigrationContext }) => {
+        await addIndexOnce(context, `${context.tableName}_record_history`, {
+          fields: ['collection', { name: 'record_id', length: 191 }, 'timestamp', 'id'],
+        });
+
+        if (DIALECTS_WITHOUT_PARTIAL_INDEXES.has(context.queryInterface.sequelize.getDialect())) {
+          return;
+        }
+
+        // The life boundary lookup keeps a record's `create` and `delete` rows only: without this,
+        // a record updated 100k times is read whole to find its one `create`.
+        await addIndexOnce(context, `${context.tableName}_life_boundaries`, {
+          fields: ['collection', { name: 'record_id', length: 191 }, 'timestamp', 'id'],
+          where: { operation: ['create', 'delete'] },
+        });
+      },
+      down: async ({ context }: { context: MigrationContext }) => {
+        const table = { tableName: context.tableName, schema: context.schema };
+        const existing = await indexNames(context.queryInterface, table, context.transaction);
+
+        for (const name of [
+          `${context.tableName}_life_boundaries`,
+          `${context.tableName}_record_history`,
+        ]) {
+          if (existing.has(name)) {
+            // eslint-disable-next-line no-await-in-loop
+            await context.queryInterface.removeIndex(table, name, {
+              transaction: context.transaction,
+            });
+          }
+        }
+      },
+    },
   ];
 }
 

packages/agent/src/audit-trail/record-visibility.ts

@@ -77,9 +77,13 @@ export default async function checkRecordVisibility(
  * The record can be deleted — or moved out of the caller's permission scope — between the check
  * that authorized the request and the audit read that answers it: the audit trail lives in its own
  * database, often its own engine, so no single snapshot spans both. Re-reads once the rows are in
- * hand, and returns null when there is nothing to re-read: a caller with no scope has nothing to
- * withhold, and a record already gone at the first check has already had the withholding applied —
- * an id recreated in the meantime would only widen what is served, never what is hidden.
+ * hand, and returns null only for a caller with no scope, who has nothing to withhold.
+ *
+ * Both reads count, and neither can clear the other, as in agent-ruby. The first is the only one that
+ * saw the record whose rows these are, so an id freed by a delete and taken by another record since
+ * answers for itself, never for the earlier life: gone at either read means gone. The second is the
+ * only one that sees a record deleted since, and the one that answers 404 when the id now belongs to
+ * a record this caller cannot read — even if it was already gone at the first.
  */
 export async function recheckRecordVisibility(
   collection: Collection,
@@ -88,7 +92,9 @@ export async function recheckRecordVisibility(
   permissionScope: ConditionTree | null,
   wasGoneEntirely: boolean,
 ): Promise<RecordVisibility | null> {
-  if (!permissionScope || wasGoneEntirely) return null;
+  if (!permissionScope) return null;
 
-  return checkRecordVisibility(collection, packedId, context, permissionScope);
+  const now = await checkRecordVisibility(collection, packedId, context, permissionScope);
+
+  return { visible: now.visible, goneEntirely: wasGoneEntirely || now.goneEntirely };
 }