feat(audit-trail): record the id a primary key moved from [PRD-1321]

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

@@ -86,6 +86,7 @@ The `forest.audit_logs` table has one row per audited change:
 | `operation`          | `create` / `update` / `delete` / `action` / `action_failed`        |
 | `collection`         | audited collection name                                            |
 | `record_id`          | packed record id (primary keys joined with `\|`); `null` for a `create` row still `pending` (the record's id isn't assigned yet) |
+| `previous_record_id` | the id the row was filed under before a confirmed `update`, set whether or not the key moved; `null` on every other operation and on any row written before this column existed. Internal: never served to a client |
 | `user_id`            | id of the Forest user who made the change                          |
 | `user_first_name`    | that user's first name, denormalized at write time                 |
 | `user_last_name`     | that user's last name, denormalized at write time                  |

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

@@ -480,6 +480,10 @@ function instrumentCollection(
         return recorder.confirm(pendingId, {
           operation: 'update',
           recordId: toPackedRecordId(updated, primaryKeys),
+          // Written whether or not the key moved. Only a value here lets the route trust that the
+          // previous side's id is known: a null has to keep meaning "this row predates the column",
+          // or an old row whose key did move would be judged by the id it moved to.
+          previousRecordId: toPackedRecordId(record, primaryKeys),
           previousValues: redactValues(previousValues, redactedFields),
           newValues: redactValues(newValues, redactedFields),
         });

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

@@ -29,6 +29,12 @@ function qualifiedMigrationName(schema: string | undefined, tableName: string):
     : `${tableName}:001-create-audit-logs`;
 }
 
+function qualifiedMigrationName002(schema: string | undefined, tableName: string): string {
+  return schema
+    ? `${schema}.${tableName}:002-add-previous-record-id`
+    : `${tableName}:002-add-previous-record-id`;
+}
+
 // 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
@@ -198,6 +204,32 @@ function buildMigrations(schema: string | undefined, tableName: string) {
         );
       },
     },
+    {
+      // Its own migration rather than a column added to 001: the table has shipped, so a database
+      // out there has already recorded 001 as applied and would never see the edit.
+      name: qualifiedMigrationName002(schema, tableName),
+      up: async ({ context }: { context: MigrationContext }) => {
+        const table = { tableName: context.tableName, schema: context.schema };
+        const existing = await columnNames(context.queryInterface, table, context.transaction);
+
+        // Idempotent for the same reason 001 is: a process losing a concurrent-boot race retries.
+        if (existing.has('previous_record_id')) return;
+
+        await context.queryInterface.addColumn(
+          table,
+          'previous_record_id',
+          { type: DataTypes.TEXT, allowNull: true },
+          { transaction: context.transaction },
+        );
+      },
+      down: async ({ context }: { context: MigrationContext }) => {
+        await context.queryInterface.removeColumn(
+          { tableName: context.tableName, schema: context.schema },
+          'previous_record_id',
+          { transaction: context.transaction },
+        );
+      },
+    },
   ];
 }
 

packages/agent/src/audit-trail/sql-store.ts

@@ -37,6 +37,9 @@ export function defineAuditLogModel(
       // migration creates the column as such — this must match. Nullable: a pending create's row
       // has no id yet, since the record doesn't exist until the write resolves.
       recordId: { type: DataTypes.TEXT, allowNull: true },
+      // Set on every confirmed update, so a null distinguishes a row older than the column from
+      // one whose key held still. TEXT for the same reason as `recordId`.
+      previousRecordId: { type: DataTypes.TEXT, allowNull: true },
       userId: { type: DataTypes.INTEGER, allowNull: true },
       // Denormalised from the caller at write time — who acted then, not who holds that id today.
       userFirstName: { type: DataTypes.TEXT, allowNull: true },
@@ -230,6 +233,7 @@ export function fromRow(row: Model): AuditRecord {
     operation: plain.operation as AuditRecord['operation'],
     collection: plain.collection as string,
     recordId: (plain.recordId as string) ?? null,
+    previousRecordId: (plain.previousRecordId as string) ?? null,
     userId: plain.userId as number,
     userFirstName: (plain.userFirstName as string) ?? null,
     userLastName: (plain.userLastName as string) ?? null,

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

@@ -11,6 +11,13 @@ export type AuditRecord = {
   collection: string;
   /** Null for a pending create — the record's primary key isn't assigned yet. */
   recordId: string | null;
+  /**
+   * The id this record was filed under before a confirmed update, written whether or not the key
+   * moved. Null therefore means "written before this column existed", not "the key held still": a
+   * row from an earlier agent cannot claim its id answers for the previous side of an update.
+   * Internal — it is how the agent follows a record across a rename, never served to a client.
+   */
+  previousRecordId: string | null;
   userId: number;
   /** Denormalised from the caller at write time: who acted then, not who holds that id today. */
   userFirstName: string | null;
@@ -30,13 +37,18 @@ export type AuditRecord = {
 };
 
 /** The subset known before the write runs, when the pending row is first inserted. */
-export type PendingAuditRecord = Omit<AuditRecord, 'id' | 'status'>;
+export type PendingAuditRecord = Omit<AuditRecord, 'id' | 'status' | 'previousRecordId'> &
+  Partial<Pick<AuditRecord, 'previousRecordId'>>;
 
-/** What `confirm` updates once the write (or action) has resolved. */
+/**
+ * What `confirm` updates once the write (or action) has resolved. `previousRecordId` is optional
+ * because only an update has a previous side to file under an id of its own.
+ */
 export type AuditRecordConfirmation = Pick<
   AuditRecord,
   'operation' | 'recordId' | 'previousValues' | 'newValues'
->;
+> &
+  Partial<Pick<AuditRecord, 'previousRecordId'>>;
 
 export type AuditHistoryQuery = {
   collection: string;

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

@@ -145,22 +145,40 @@ export default function withholdOutsidePermissionScope(
   return entries.map(entry => {
     if (entry.operation === 'action' || entry.operation === 'action_failed') return entry;
 
-    // Decoded once per row rather than once per side: the two sides read the same id, and a row
-    // whose id no longer decodes should say so once.
-    const decoded = decodePrimaryKeys(entry.recordId, withholding.collection, withholding.logger);
-
-    const side = (values: Record<string, unknown>, idAnswersForSide: IdAnswersForSide) =>
+    const side = (
+      values: Record<string, unknown>,
+      decoded: ReturnType<typeof decodePrimaryKeys>,
+      idAnswersForSide: IdAnswersForSide,
+    ) =>
       permissionScopeAccepts(answerableSnapshot(values, decoded, idAnswersForSide), withholding)
         ? values ?? {}
         : {};
 
+    // An update that recorded where it came from is judged side by side: the previous state
+    // against the id it was filed under then, the new state against the id it ended up with. A row
+    // without that column is older than it, so its id still answers for the new side alone.
+    // `?? null` first: the column is optional on the write types, so an absent one has to read as
+    // unknown exactly like a null, or a row that never recorded it would answer from the id it
+    // ended up with.
+    const previousRecordId = entry.previousRecordId ?? null;
+    const previousIsKnown = entry.operation !== 'update' || previousRecordId !== null;
+
+    // Decoded once per distinct id, not once per side: a row whose id no longer decodes should say
+    // so once, and the two sides read the same id unless the key actually moved.
+    const decoded = decodePrimaryKeys(entry.recordId, withholding.collection, withholding.logger);
+    const decodedPrevious =
+      previousRecordId === null
+        ? decoded
+        : decodePrimaryKeys(previousRecordId, withholding.collection, withholding.logger);
+
+    // A pending update is filed under the id the record had before the write, which says nothing
+    // about the state it was moving to, so the new side answers only with what it captured.
+    const newIsKnown = entry.status !== 'pending';
+
     return {
       ...entry,
-      // The row is filed under the identity the record ended up with, so its id answers for the
-      // new side of an update and for a create or a delete — never for what an update moved away
-      // from.
-      previousValues: side(entry.previousValues, entry.operation !== 'update'),
-      newValues: side(entry.newValues, true),
+      previousValues: side(entry.previousValues, decodedPrevious, previousIsKnown),
+      newValues: side(entry.newValues, decoded, newIsKnown),
     };
   });
 }

packages/agent/src/routes/access/audit-trail.ts

@@ -174,7 +174,8 @@ export default class AuditTrailRoute extends CollectionRoute {
       permissionScope && gone ? this.withhold(rawData, permissionScope, context) : rawData;
 
     context.response.body = {
-      data,
+      // `previousRecordId` stays out here too: the served shape is the same whichever path built it.
+      data: data.map(({ previousRecordId, ...served }) => served),
       meta: { count, ...(availableUsers && { availableUsers }) },
     };
   }
@@ -186,7 +187,7 @@ export default class AuditTrailRoute extends CollectionRoute {
     permissionScope: ConditionTree,
     context: Context,
   ): Promise<{
-    data: AuditRecord[];
+    data: Array<Omit<AuditRecord, 'previousRecordId'>>;
     meta: { count: number; availableUsers?: AuditUserSummary[] };
   }> {
     const { store } = this.options.auditTrail;
@@ -226,7 +227,9 @@ export default class AuditTrailRoute extends CollectionRoute {
     }
 
     return {
-      data: page,
+      // `previousRecordId` stays out: it is how the agent follows a record across a rename, not
+      // something a client reads.
+      data: page.map(({ previousRecordId, ...served }) => served),
       meta: { count, ...(isFirstFetch && { availableUsers: [...authors.values()] }) },
     };
   }

packages/agent/test/audit-trail/in-memory-store.test.ts

@@ -9,6 +9,7 @@ const record = (
   operation: 'update',
   collection: 'accounts',
   recordId: '1',
+  previousRecordId: null,
   userId: 1,
   userFirstName: null,
   userLastName: null,

packages/agent/test/audit-trail/in-memory-store.ts

@@ -30,7 +30,7 @@ export default class InMemoryAuditStore implements AuditStore {
   async insertPending(record: PendingAuditRecord): Promise<number> {
     const id = this.nextId;
     this.nextId += 1;
-    this.records.push({ ...record, id, status: 'pending' });
+    this.records.push({ previousRecordId: null, ...record, id, status: 'pending' });
 
     return id;
   }

packages/agent/test/audit-trail/instrument.test.ts

@@ -70,7 +70,7 @@
     },
   };
 
   const fire = (key: string, context: Record<string, unknown>) => handlers.get(key)!(context);
 
   return { collection, handlers, list, fire, records: listResult };
 }
@@ -702,12 +702,32 @@
       expect(sink).toHaveBeenCalledWith(
         expect.objectContaining({
           recordId: 'new-slug',
+          // What the route needs to judge the previous side by the identity it actually had.
+          previousRecordId: 'old-slug',
           previousValues: { slug: 'old-slug' },
           newValues: { slug: 'new-slug' },
         }),
       );
     });
 
+    it('records where a row came from even when the key held still', async () => {
+      const sink = jest.fn();
+      const accounts = fakeCollection('accounts', [{ id: 1, name: 'Acme', amount: 10 }]);
+      register([accounts], { sink });
+
+      await runUpdate(accounts, {
+        caller: makeCaller(),
+        patch: { name: 'Acme Inc' },
+        after: [{ id: 1, name: 'Acme Inc', amount: 10 }],
+      });
+
+      // Not only on a move: a null has to keep meaning "written before this column existed", or a
+      // row from an older agent whose key did move would be judged by the id it moved to.
+      expect(sink).toHaveBeenCalledWith(
+        expect.objectContaining({ recordId: '1', previousRecordId: '1' }),
+      );
+    });
+
     it('still records an update that changes a field the update was itself filtered on', async () => {
       const sink = jest.fn();
       const accounts = fakeCollection('accounts', [

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

@@ -43,6 +43,7 @@ describe('runAuditMigrations (sqlite)', () => {
       'id',
       'new_values',
       'operation',
+      'previous_record_id',
       'previous_values',
       'record_id',
       'status',
@@ -68,7 +69,10 @@ describe('runAuditMigrations (sqlite)', () => {
     // sqlite has no real schema/catalog separation: Sequelize represents a schema-qualified table
     // as a single literal identifier joining schema and table name with a dot.
     const [applied] = await sequelize.query('SELECT name FROM "forest.audit_logs_migration"');
-    expect(applied).toEqual([{ name: 'forest.audit_logs:001-create-audit-logs' }]);
+    expect(applied).toEqual([
+      { name: 'forest.audit_logs:001-create-audit-logs' },
+      { name: 'forest.audit_logs:002-add-previous-record-id' },
+    ]);
 
     await sequelize.close();
   });
@@ -79,7 +83,10 @@ describe('runAuditMigrations (sqlite)', () => {
     await runAuditMigrations(sequelize, { tableName: 'audit_logs' });
 
     const [applied] = await sequelize.query('SELECT name FROM "audit_logs_migration"');
-    expect(applied).toEqual([{ name: 'audit_logs:001-create-audit-logs' }]);
+    expect(applied).toEqual([
+      { name: 'audit_logs:001-create-audit-logs' },
+      { name: 'audit_logs:002-add-previous-record-id' },
+    ]);
 
     await sequelize.close();
   });
@@ -163,7 +170,7 @@ describe('runAuditMigrations (sqlite)', () => {
     ).resolves.toBeUndefined();
 
     const [applied] = await sequelize.query('SELECT name FROM "audit_logs_migration"');
-    expect(applied).toHaveLength(1);
+    expect(applied).toHaveLength(2);
 
     await sequelize.close();
   });

packages/agent/test/routes/access/audit-trail.test.ts

@@ -1513,6 +1513,65 @@ describe('AuditTrailRoute', () => {
         ]);
       });
 
+      // What PRD-1321 buys back: with the id the row was filed under before the move recorded, the
+      // previous side is judged by that id instead of being withheld for want of an answer.
+      test('judges a moved key against the id the previous side carried', async () => {
+        const data = await historyUnder(new ConditionTreeLeaf('id', 'Equal', 2), [
+          {
+            operation: 'update',
+            recordId: '9',
+            previousRecordId: '2',
+            previousValues: { id: REDACTED, secret: 'was in scope then' },
+            newValues: { id: REDACTED, secret: 'out of scope now' },
+          },
+        ]);
+
+        expect(data).toEqual([
+          {
+            operation: 'update',
+            recordId: '9',
+            previousValues: { id: REDACTED, secret: 'was in scope then' },
+            newValues: {},
+          },
+        ]);
+      });
+
+      test("never serves the id a move came from, which is the agent's own bookkeeping", async () => {
+        const data = await historyUnder(new ConditionTreeLeaf('id', 'Equal', 2), [
+          {
+            operation: 'update',
+            recordId: '9',
+            previousRecordId: '2',
+            previousValues: { ownerId: 2 },
+            newValues: { ownerId: 2 },
+          },
+        ]);
+
+        expect(data[0]).not.toHaveProperty('previousRecordId');
+      });
+
+      test("withholds a pending update's new side, which its id cannot speak for", async () => {
+        const data = await historyUnder(new ConditionTreeLeaf('id', 'Equal', 2), [
+          {
+            operation: 'update',
+            recordId: '2',
+            status: 'pending',
+            previousValues: { id: 2, secret: 'before' },
+            newValues: { id: REDACTED, secret: 'after' },
+          },
+        ]);
+
+        expect(data).toEqual([
+          {
+            operation: 'update',
+            recordId: '2',
+            status: 'pending',
+            previousValues: { id: 2, secret: 'before' },
+            newValues: {},
+          },
+        ]);
+      });
+
       test('withholds the values when the scope names an inherited property of the snapshot', async () => {
         const data = await historyUnder(new ConditionTreeLeaf('toString', 'NotEqual', 'private'), [
           { operation: 'delete', recordId: '2', previousValues: { ownerId: 1, secret: 'shh' } },