From 34fc1b1c46d040342e36107aa4bbac1c1913d6b8 Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Wed, 30 Sep 2026 17:55:39 +0200 Subject: [PATCH 1/4] feat(audit-trail): record the id a primary key moved from, and judge each side by it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An update carries two states but files under one id, so a writable primary key that is also redacted left the previous side with nothing to answer a permission scope: #1909 withheld it rather than judging it by the id the record ended up with. Correct, and lossy — a caller squarely in scope saw nothing. The row now carries `previous_record_id`, written on every confirmed update, and each side is judged against the id it actually had. Written whether or not the key moved, which is where this departs from agent-ruby#394: that table had never shipped, so a null there can only mean the key held still. Here 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 — the leak #1909 closed. For the same reason the column arrives as its own migration rather than an edit to 001. A pending update's new side still answers with what it captured: the row is filed under the id the record had before the write, which says nothing about the state it was moving to. Co-Authored-By: Claude Opus 5 (1M context) --- packages/agent/src/audit-trail/README.md | 1 + packages/agent/src/audit-trail/instrument.ts | 4 ++ packages/agent/src/audit-trail/migrations.ts | 32 ++++++++++ packages/agent/src/audit-trail/sql-store.ts | 4 ++ packages/agent/src/audit-trail/types.ts | 18 +++++- packages/agent/src/audit-trail/withhold.ts | 38 ++++++++---- .../agent/src/routes/access/audit-trail.ts | 6 +- .../test/audit-trail/in-memory-store.test.ts | 1 + .../agent/test/audit-trail/in-memory-store.ts | 2 +- .../agent/test/audit-trail/instrument.test.ts | 20 +++++++ .../agent/test/audit-trail/migrations.test.ts | 7 ++- .../test/routes/access/audit-trail.test.ts | 59 +++++++++++++++++++ 12 files changed, 174 insertions(+), 18 deletions(-) diff --git a/packages/agent/src/audit-trail/README.md b/packages/agent/src/audit-trail/README.md index 70d4b9f0d3..cdb2eb21ea 100644 --- a/packages/agent/src/audit-trail/README.md +++ b/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 | diff --git a/packages/agent/src/audit-trail/instrument.ts b/packages/agent/src/audit-trail/instrument.ts index 79572c28f5..7921b0e69c 100644 --- a/packages/agent/src/audit-trail/instrument.ts +++ b/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), }); diff --git a/packages/agent/src/audit-trail/migrations.ts b/packages/agent/src/audit-trail/migrations.ts index da82101bc6..4668ec022c 100644 --- a/packages/agent/src/audit-trail/migrations.ts +++ b/packages/agent/src/audit-trail/migrations.ts @@ -35,6 +35,12 @@ 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-add-previous-record-id` + : `${tableName}:003-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 @@ -240,6 +246,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: qualifiedMigrationName003(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 }, + ); + }, + }, ]; } diff --git a/packages/agent/src/audit-trail/sql-store.ts b/packages/agent/src/audit-trail/sql-store.ts index 69e93846fc..ead827c1cb 100644 --- a/packages/agent/src/audit-trail/sql-store.ts +++ b/packages/agent/src/audit-trail/sql-store.ts @@ -38,6 +38,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 }, @@ -279,6 +282,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, diff --git a/packages/agent/src/audit-trail/types.ts b/packages/agent/src/audit-trail/types.ts index 75c9b1c468..820741c643 100644 --- a/packages/agent/src/audit-trail/types.ts +++ b/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; +export type PendingAuditRecord = Omit & + Partial>; -/** 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>; export type AuditHistoryQuery = { collection: string; diff --git a/packages/agent/src/audit-trail/withhold.ts b/packages/agent/src/audit-trail/withhold.ts index 1bea817644..4d89222ae8 100644 --- a/packages/agent/src/audit-trail/withhold.ts +++ b/packages/agent/src/audit-trail/withhold.ts @@ -195,22 +195,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, idAnswersForSide: IdAnswersForSide) => + const side = ( + values: Record, + decoded: ReturnType, + 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), }; }); } diff --git a/packages/agent/src/routes/access/audit-trail.ts b/packages/agent/src/routes/access/audit-trail.ts index 456ee5e331..94edab757a 100644 --- a/packages/agent/src/routes/access/audit-trail.ts +++ b/packages/agent/src/routes/access/audit-trail.ts @@ -106,7 +106,9 @@ export default class AuditTrailRoute extends CollectionRoute { }); context.response.body = { - data: matched.page, + // `previousRecordId` stays out: it is how the agent follows a record across a rename, not + // something a client reads. + data: matched.page.map(({ previousRecordId, ...served }) => served), meta: { count: matched.count, ...(isFirstFetch && { availableUsers: [...matched.authors.values()] }), @@ -165,7 +167,7 @@ export default class AuditTrailRoute extends CollectionRoute { permissionScope && gone ? this.withhold(rawData, permissionScope, context) : rawData; context.response.body = { - data, + data: data.map(({ previousRecordId, ...served }) => served), meta: { count, ...(availableUsers && { availableUsers }) }, }; } diff --git a/packages/agent/test/audit-trail/in-memory-store.test.ts b/packages/agent/test/audit-trail/in-memory-store.test.ts index 286dd4f60f..b219f14d79 100644 --- a/packages/agent/test/audit-trail/in-memory-store.test.ts +++ b/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, diff --git a/packages/agent/test/audit-trail/in-memory-store.ts b/packages/agent/test/audit-trail/in-memory-store.ts index 2e06bbc95e..802c9f8666 100644 --- a/packages/agent/test/audit-trail/in-memory-store.ts +++ b/packages/agent/test/audit-trail/in-memory-store.ts @@ -31,7 +31,7 @@ export default class InMemoryAuditStore implements AuditStore { async insertPending(record: PendingAuditRecord): Promise { 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; } diff --git a/packages/agent/test/audit-trail/instrument.test.ts b/packages/agent/test/audit-trail/instrument.test.ts index 9c7d2128c5..9f721469a4 100644 --- a/packages/agent/test/audit-trail/instrument.test.ts +++ b/packages/agent/test/audit-trail/instrument.test.ts @@ -702,12 +702,32 @@ describe('auditTrail plugin', () => { 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', [ diff --git a/packages/agent/test/audit-trail/migrations.test.ts b/packages/agent/test/audit-trail/migrations.test.ts index 10051ec769..384a011177 100644 --- a/packages/agent/test/audit-trail/migrations.test.ts +++ b/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', @@ -71,6 +72,7 @@ describe('runAuditMigrations (sqlite)', () => { expect(applied).toEqual([ { name: 'forest.audit_logs:001-create-audit-logs' }, { name: 'forest.audit_logs:002-index-timestamp-id' }, + { name: 'forest.audit_logs:003-add-previous-record-id' }, ]); await sequelize.close(); @@ -85,6 +87,7 @@ describe('runAuditMigrations (sqlite)', () => { expect(applied).toEqual([ { name: 'audit_logs:001-create-audit-logs' }, { name: 'audit_logs:002-index-timestamp-id' }, + { name: 'audit_logs:003-add-previous-record-id' }, ]); await sequelize.close(); @@ -169,7 +172,7 @@ describe('runAuditMigrations (sqlite)', () => { ).resolves.toBeUndefined(); const [applied] = await sequelize.query('SELECT name FROM "audit_logs_migration"'); - expect(applied).toHaveLength(2); + expect(applied).toHaveLength(3); await sequelize.close(); }); @@ -255,7 +258,7 @@ describe('runAuditMigrations (sqlite)', () => { const umzug = buildUmzug(sequelize, { tableName: 'audit_logs' }); await umzug.up(); - await umzug.down(); + await umzug.down({ step: 2 }); expect(await indexOf(sequelize)).toBeUndefined(); diff --git a/packages/agent/test/routes/access/audit-trail.test.ts b/packages/agent/test/routes/access/audit-trail.test.ts index 0b3e4b61f7..bd737cdf4e 100644 --- a/packages/agent/test/routes/access/audit-trail.test.ts +++ b/packages/agent/test/routes/access/audit-trail.test.ts @@ -1628,6 +1628,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' } }, From 1f850de76aca2ceac115fdbceaf4bb1ea82be543 Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Wed, 7 Oct 2026 15:50:03 +0200 Subject: [PATCH 2/4] feat(audit-trail): announce the timeline's authors on its first page The project-level author filter could only offer activity-log authors: the cross-collection route answered no availableUsers. It now carries them on the first page (no before), like the per-record route, through an optional store method, listTimelineUsers. Authors are not detail values, so the admin gate leaves them in. Stacking on the timeline route also exposed previousRecordId on its rows; it is stripped there as on the per-record paths. Co-Authored-By: Claude Opus 5.5 --- packages/agent/src/audit-trail/README.md | 7 ++ packages/agent/src/audit-trail/sql-store.ts | 69 +++++++++++------- packages/agent/src/audit-trail/types.ts | 8 +++ .../src/routes/access/audit-trail-timeline.ts | 18 +++-- .../agent/test/audit-trail/in-memory-store.ts | 11 ++- .../agent/test/audit-trail/sql-store.test.ts | 31 ++++++++ .../access/audit-trail-timeline.test.ts | 71 ++++++++++++++++++- 7 files changed, 181 insertions(+), 34 deletions(-) diff --git a/packages/agent/src/audit-trail/README.md b/packages/agent/src/audit-trail/README.md index cdb2eb21ea..65d10f9827 100644 --- a/packages/agent/src/audit-trail/README.md +++ b/packages/agent/src/audit-trail/README.md @@ -383,7 +383,14 @@ across pages rather than looping. Feed the two values back verbatim; don't synth must always add at least the last row's id to the exclusions, so a cursor that would come back unchanged ends the walk instead of repeating the page forever. +**Authors.** On the first page only (no `before`), `meta` also carries `availableUsers`: the +distinct authors matching the active filters across the collections queried, independent of the +cursor, in the per-record route's shape. Later pages omit the key rather than send `[]`, so a client +keeps the list it already saw. Authors are not detail values, so the admin rule above leaves them in. + A custom `AuditStore` that doesn't implement `listTimeline` simply doesn't get this route mounted. +One that implements `listTimeline` without `listTimelineUsers` serves the rows with no +`availableUsers`. ### `GET /forest/_audit-trail/correlation/{correlationKey}` diff --git a/packages/agent/src/audit-trail/sql-store.ts b/packages/agent/src/audit-trail/sql-store.ts index ead827c1cb..89c3356467 100644 --- a/packages/agent/src/audit-trail/sql-store.ts +++ b/packages/agent/src/audit-trail/sql-store.ts @@ -5,6 +5,7 @@ import type { AuditStorageOptions, AuditStore, AuditTimelineQuery, + AuditUserSummary, PendingAuditRecord, } from './types'; import type { Model, ModelStatic } from 'sequelize'; @@ -246,7 +247,7 @@ function buildTimelineWhereClause( startTimestamp, endTimestamp, search, - }: AuditTimelineQuery, + }: Omit, sequelize: Sequelize, ): Record { const where: Record = { collection: { [Op.in]: collections } }; @@ -272,6 +273,38 @@ function buildTimelineWhereClause( return where; } +async function listAuthors( + model: ModelStatic, + where: Record, +): Promise { + // MAX() rather than a bare column: grouping by `user_id` alone is invalid in strict SQL + // unless every selected column is either grouped or aggregated. + const rows = (await model.findAll({ + where, + attributes: [ + 'userId', + [Sequelize.fn('MAX', Sequelize.col('user_first_name')), 'userFirstName'], + [Sequelize.fn('MAX', Sequelize.col('user_last_name')), 'userLastName'], + [Sequelize.fn('MAX', Sequelize.col('user_email')), 'userEmail'], + ], + group: ['userId'], + raw: true, + transaction: null, + })) as unknown as Array<{ + userId: number; + userFirstName: string | null; + userLastName: string | null; + userEmail: string | null; + }>; + + return rows.map(row => ({ + id: row.userId, + firstName: row.userFirstName ?? null, + lastName: row.userLastName ?? null, + email: row.userEmail ?? null, + })); +} + export function fromRow(row: Model): AuditRecord { const plain = row.get({ plain: true }) as Record; const { timestamp } = plain; @@ -424,32 +457,14 @@ export function createSqlAuditStore(options: AuditStorageOptions): { async listDistinctUsers(query) { const { model, connection } = await init(); - // MAX() rather than a bare column: grouping by `user_id` alone is invalid in strict SQL - // unless every selected column is either grouped or aggregated. - const rows = (await model.findAll({ - where: buildHistoryWhereClause(query as AuditHistoryQuery, connection), - attributes: [ - 'userId', - [Sequelize.fn('MAX', Sequelize.col('user_first_name')), 'userFirstName'], - [Sequelize.fn('MAX', Sequelize.col('user_last_name')), 'userLastName'], - [Sequelize.fn('MAX', Sequelize.col('user_email')), 'userEmail'], - ], - group: ['userId'], - raw: true, - transaction: null, - })) as unknown as Array<{ - userId: number; - userFirstName: string | null; - userLastName: string | null; - userEmail: string | null; - }>; - - return rows.map(row => ({ - id: row.userId, - firstName: row.userFirstName ?? null, - lastName: row.userLastName ?? null, - email: row.userEmail ?? null, - })); + return listAuthors(model, buildHistoryWhereClause(query as AuditHistoryQuery, connection)); + }, + async listTimelineUsers(query) { + if (!query.collections.length) return []; + + const { model, connection } = await init(); + + return listAuthors(model, buildTimelineWhereClause(query, connection)); }, async listByCorrelation({ collection, recordId, correlationKey }) { const { model } = await init(); diff --git a/packages/agent/src/audit-trail/types.ts b/packages/agent/src/audit-trail/types.ts index 820741c643..8abdd32190 100644 --- a/packages/agent/src/audit-trail/types.ts +++ b/packages/agent/src/audit-trail/types.ts @@ -152,6 +152,14 @@ export interface AuditStore { * written before this existed simply doesn't serve the project-level route. */ listTimeline?(query: AuditTimelineQuery): AuditRecord[] | Promise; + /** + * Distinct authors matching the timeline's filters, independent of its cursor. Optional like + * `listTimeline`: without it the route serves the rows but no author list. An empty + * `collections` must match nothing. + */ + listTimelineUsers?( + query: Omit, + ): AuditUserSummary[] | Promise; /** Distinct authors matching the query filters, independent of pagination. */ listDistinctUsers( query: Omit, diff --git a/packages/agent/src/routes/access/audit-trail-timeline.ts b/packages/agent/src/routes/access/audit-trail-timeline.ts index d4aa88aa0f..199225bc4e 100644 --- a/packages/agent/src/routes/access/audit-trail-timeline.ts +++ b/packages/agent/src/routes/access/audit-trail-timeline.ts @@ -54,16 +54,26 @@ export default class AuditTrailTimelineRoute extends BaseRoute { const collections = await this.readableCollections(context); const { store } = this.options.auditTrail; + // Authors on the first page only, like the per-record route and Forest's activity-logs route: + // they do not change along the walk, and later pages omit the key rather than send `[]`. // One row over the page: the only way to know whether a further page exists without a count. - const fetched = collections.length - ? await store.listTimeline({ ...filters, ...cursor, collections, limit: limit + 1 }) - : []; + const [fetched, availableUsers] = await Promise.all([ + collections.length + ? store.listTimeline({ ...filters, ...cursor, collections, limit: limit + 1 }) + : [], + !cursor.before && store.listTimelineUsers + ? store.listTimelineUsers({ ...filters, collections }) + : undefined, + ]); const page = fetched.slice(0, limit); context.response.body = { - 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: { cursor: fetched.length > limit ? AuditTrailTimelineRoute.nextCursor(page, cursor) : null, + ...(availableUsers && { availableUsers }), }, }; } diff --git a/packages/agent/test/audit-trail/in-memory-store.ts b/packages/agent/test/audit-trail/in-memory-store.ts index 802c9f8666..1df86706ea 100644 --- a/packages/agent/test/audit-trail/in-memory-store.ts +++ b/packages/agent/test/audit-trail/in-memory-store.ts @@ -112,10 +112,19 @@ export default class InMemoryAuditStore implements AuditStore { .slice(0, limit); } + listTimelineUsers( + query: Omit, + ): AuditUserSummary[] { + return InMemoryAuditStore.authorsOf(this.listTimeline({ ...query, limit: Infinity })); + } + listDistinctUsers( query: Omit, ): AuditUserSummary[] { - const matches = this.matching({ ...query, order: 'asc' }); + return InMemoryAuditStore.authorsOf(this.matching({ ...query, order: 'asc' })); + } + + private static authorsOf(matches: AuditRecord[]): AuditUserSummary[] { const byUser = new Map(); for (const record of matches) { diff --git a/packages/agent/test/audit-trail/sql-store.test.ts b/packages/agent/test/audit-trail/sql-store.test.ts index bb0a56f615..df1f79ecf8 100644 --- a/packages/agent/test/audit-trail/sql-store.test.ts +++ b/packages/agent/test/audit-trail/sql-store.test.ts @@ -964,6 +964,37 @@ describe('createSqlAuditStore (sqlite round-trip)', () => { await close(); }); + + describe('listTimelineUsers', () => { + it('returns the distinct authors on the allowed collections, under the timeline filters', async () => { + const { store, close } = createSqlAuditStore({ connectionString: 'sqlite::memory:' }); + + await seed(store, record({ collection: 'accounts', userId: 1, userFirstName: 'Jane' })); + await seed(store, record({ collection: 'books', userId: 1, userFirstName: 'Jane' })); + await seed(store, record({ collection: 'books', userId: 2, operation: 'delete' })); + await seed(store, record({ collection: 'secrets', userId: 3, userFirstName: 'Hidden' })); + + const users = await store.listTimelineUsers({ + collections: ['accounts', 'books'], + operations: ['update'], + }); + + expect(users).toEqual([ + { id: 1, firstName: 'Jane', lastName: 'Doe', email: 'jane.doe@forest.dev' }, + ]); + + await close(); + }); + + it('matches nothing when no collection is allowed', async () => { + const { store, close } = createSqlAuditStore({ connectionString: 'sqlite::memory:' }); + await seed(store, record({ collection: 'accounts', userId: 1 })); + + expect(await store.listTimelineUsers({ collections: [] })).toEqual([]); + + await close(); + }); + }); }); describe('fieldsChangedCondition', () => { diff --git a/packages/agent/test/routes/access/audit-trail-timeline.test.ts b/packages/agent/test/routes/access/audit-trail-timeline.test.ts index 934c3ce00a..c7d394d2e1 100644 --- a/packages/agent/test/routes/access/audit-trail-timeline.test.ts +++ b/packages/agent/test/routes/access/audit-trail-timeline.test.ts @@ -18,7 +18,7 @@ describe('AuditTrailTimelineRoute', () => { ...patch, }); - const setup = (timeline: unknown[] = []) => { + const setup = (timeline: unknown[] = [], users?: unknown[]) => { const services = factories.forestAdminHttpDriverServices.build(); const dataSource = factories.dataSource.buildWithCollections([ factories.collection.build({ @@ -34,7 +34,10 @@ describe('AuditTrailTimelineRoute', () => { }), }), ]); - const store = { listTimeline: jest.fn().mockResolvedValue(timeline) }; + const store = { + listTimeline: jest.fn().mockResolvedValue(timeline), + ...(users && { listTimelineUsers: jest.fn().mockResolvedValue(users) }), + }; const options = factories.forestAdminHttpDriverOptions.build({ auditTrail: { connectionString: 'sqlite::memory:', store } as never, }); @@ -145,6 +148,70 @@ describe('AuditTrailTimelineRoute', () => { expect(store.listTimeline).not.toHaveBeenCalled(); }); + test('never serves the id a record moved from, which is internal', async () => { + const { route } = setup([row(1, { previousRecordId: '1' })]); + const context = contextWith(); + + await route.handleTimeline(context); + + expect((context.response.body as { data: object[] }).data[0]).not.toHaveProperty( + 'previousRecordId', + ); + }); + + describe('available users', () => { + const jane = { id: 1, firstName: 'Jane', lastName: 'Doe', email: 'jane@forest.dev' }; + + test('announces the authors on the first page, with the filters and readable collections', async () => { + const { store, route } = setup([row(1)], [jane]); + const context = contextWith({ userIds: '1', operations: 'update' }); + + await route.handleTimeline(context); + + expect(store.listTimelineUsers).toHaveBeenCalledWith( + expect.objectContaining({ + collections: ['books', 'authors'], + userIds: [1], + operations: ['update'], + }), + ); + expect((context.response.body as { meta: object }).meta).toEqual({ + cursor: null, + availableUsers: [jane], + }); + }); + + test('omits the key on a later page rather than sending an empty list', async () => { + const { store, route } = setup([row(1)], [jane]); + const context = contextWith({ before: '2026-01-05T00:00:00.000Z' }); + + await route.handleTimeline(context); + + expect(store.listTimelineUsers).not.toHaveBeenCalled(); + expect((context.response.body as { meta: object }).meta).not.toHaveProperty('availableUsers'); + }); + + test('omits the key when the store cannot list the authors', async () => { + const { route } = setup([row(1)]); + const context = contextWith(); + + await route.handleTimeline(context); + + expect((context.response.body as { meta: object }).meta).not.toHaveProperty('availableUsers'); + }); + + test('still names the authors to a non-admin, since they are not detail values', async () => { + const { route } = setup([row(1)], [jane]); + const context = contextWith({}, 'user'); + + await route.handleTimeline(context); + + expect( + (context.response.body as { meta: { availableUsers: unknown[] } }).meta.availableUsers, + ).toEqual([jane]); + }); + }); + describe('cursor', () => { test('returns no cursor when the page is the last one', async () => { const { route } = setup([row(2), row(1)]); From 9bbed76276dc59cc041e8beee44001cb557152eb Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Wed, 7 Oct 2026 16:30:53 +0200 Subject: [PATCH 3/4] fix(audit-trail): tolerate a concurrent boot adding previous_record_id, and keep it at insert Migration 002 checked for the column, then added it. Without Postgres's advisory lock, a second agent booting at the same moment could add it in between, and the duplicate-column error failed this agent's startup. A failed addColumn now re-reads the table: a column that is there means the work is done, anything else is rethrown. toRow also keeps a previousRecordId given at insert, as the in-memory store does, so a confirm that omits it cannot make the row read as older than the column. Co-Authored-By: Claude Opus 5.5 --- packages/agent/src/audit-trail/migrations.ts | 24 ++++++++--- packages/agent/src/audit-trail/sql-store.ts | 3 ++ .../agent/test/audit-trail/migrations.test.ts | 43 +++++++++++++++++++ .../agent/test/audit-trail/sql-store.test.ts | 9 ++++ 4 files changed, 73 insertions(+), 6 deletions(-) diff --git a/packages/agent/src/audit-trail/migrations.ts b/packages/agent/src/audit-trail/migrations.ts index 4668ec022c..13ae940765 100644 --- a/packages/agent/src/audit-trail/migrations.ts +++ b/packages/agent/src/audit-trail/migrations.ts @@ -257,12 +257,24 @@ function buildMigrations(schema: string | undefined, tableName: string) { // 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 }, - ); + try { + await context.queryInterface.addColumn( + table, + 'previous_record_id', + { type: DataTypes.TEXT, allowNull: true }, + { transaction: context.transaction }, + ); + } catch (error) { + // Without Postgres's advisory lock, another agent booting at the same moment can add the + // column between the check above and this statement. Its duplicate-column error then + // means the work is done. Anything else, or a column still missing, is a real failure. + const added = await columnNames(context.queryInterface, table, context.transaction).then( + columns => columns.has('previous_record_id'), + () => false, + ); + + if (!added) throw error; + } }, down: async ({ context }: { context: MigrationContext }) => { await context.queryInterface.removeColumn( diff --git a/packages/agent/src/audit-trail/sql-store.ts b/packages/agent/src/audit-trail/sql-store.ts index 89c3356467..9459be3d8f 100644 --- a/packages/agent/src/audit-trail/sql-store.ts +++ b/packages/agent/src/audit-trail/sql-store.ts @@ -83,6 +83,9 @@ export function toRow( operation: record.operation, collection: record.collection, recordId: record.recordId, + // Optional at insert, but kept when given, as the in-memory store keeps it: a later `confirm` + // that omits it must not read as a row older than the column. + previousRecordId: record.previousRecordId ?? null, userId: record.userId, userFirstName: record.userFirstName, userLastName: record.userLastName, diff --git a/packages/agent/test/audit-trail/migrations.test.ts b/packages/agent/test/audit-trail/migrations.test.ts index 384a011177..cee5cd0a10 100644 --- a/packages/agent/test/audit-trail/migrations.test.ts +++ b/packages/agent/test/audit-trail/migrations.test.ts @@ -266,6 +266,49 @@ describe('runAuditMigrations (sqlite)', () => { }); }); + it('treats previous_record_id as added when a concurrent boot added it after the check', async () => { + const sequelize = new Sequelize('sqlite::memory:', { logging: false }); + await runAuditMigrations(sequelize, { tableName: 'audit_logs' }); + await sequelize.query( + `DELETE FROM "audit_logs_migration" WHERE name = 'audit_logs:003-add-previous-record-id'`, + ); + // The check reads the table before the other boot's column lands, so this one still adds it. + const queryInterface = sequelize.getQueryInterface(); + const describeTable = queryInterface.describeTable.bind(queryInterface); + jest.spyOn(queryInterface, 'describeTable').mockImplementationOnce(async (...args) => { + const { previous_record_id: lateColumn, ...columns } = await describeTable(...args); + + return columns; + }); + + await expect( + runAuditMigrations(sequelize, { tableName: 'audit_logs' }), + ).resolves.toBeUndefined(); + + const [applied] = await sequelize.query('SELECT name FROM "audit_logs_migration"'); + expect(applied).toHaveLength(3); + + await sequelize.close(); + }); + + it('still fails migration 003 when adding the column fails and the column is missing', async () => { + const sequelize = new Sequelize('sqlite::memory:', { logging: false }); + await runAuditMigrations(sequelize, { tableName: 'audit_logs' }); + await sequelize.query('ALTER TABLE "audit_logs" DROP COLUMN "previous_record_id"'); + await sequelize.query( + `DELETE FROM "audit_logs_migration" WHERE name = 'audit_logs:003-add-previous-record-id'`, + ); + jest + .spyOn(sequelize.getQueryInterface(), 'addColumn') + .mockRejectedValueOnce(new Error('disk full')); + + await expect(runAuditMigrations(sequelize, { tableName: 'audit_logs' })).rejects.toThrow( + 'disk full', + ); + + await sequelize.close(); + }); + it('rejects a pre-existing table sharing the name that is missing audit-trail columns', async () => { const sequelize = new Sequelize('sqlite::memory:', { logging: false }); await sequelize.getQueryInterface().createTable('audit_logs', { diff --git a/packages/agent/test/audit-trail/sql-store.test.ts b/packages/agent/test/audit-trail/sql-store.test.ts index df1f79ecf8..c13d2b0fab 100644 --- a/packages/agent/test/audit-trail/sql-store.test.ts +++ b/packages/agent/test/audit-trail/sql-store.test.ts @@ -51,6 +51,7 @@ describe('toRow', () => { operation: 'update', collection: 'accounts', recordId: '1', + previousRecordId: null, userId: 42, userFirstName: 'Jane', userLastName: 'Doe', @@ -62,6 +63,14 @@ describe('toRow', () => { status: 'done', }); }); + + it('keeps a previousRecordId given at insert, like the in-memory store', () => { + const status: AuditStatus = 'pending'; + + expect(toRow({ ...record(), previousRecordId: '7', status })).toEqual( + expect.objectContaining({ previousRecordId: '7' }), + ); + }); }); describe('ensureAuditStorage', () => { From 0e9132021b83d5647e0b9d23a821bc3cb5a256ad Mon Sep 17 00:00:00 2001 From: Olivier Bex-Chauvet Date: Thu, 8 Oct 2026 16:25:46 +0200 Subject: [PATCH 4/4] docs(audit-trail): drop the admin wording around the timeline's authors The admin gate is gone, so authors need no exemption from it. Co-Authored-By: Claude Opus 5.5 --- packages/agent/src/audit-trail/README.md | 2 +- .../test/routes/access/audit-trail-timeline.test.ts | 11 ----------- 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/packages/agent/src/audit-trail/README.md b/packages/agent/src/audit-trail/README.md index 65d10f9827..5c6c64ad1f 100644 --- a/packages/agent/src/audit-trail/README.md +++ b/packages/agent/src/audit-trail/README.md @@ -386,7 +386,7 @@ unchanged ends the walk instead of repeating the page forever. **Authors.** On the first page only (no `before`), `meta` also carries `availableUsers`: the distinct authors matching the active filters across the collections queried, independent of the cursor, in the per-record route's shape. Later pages omit the key rather than send `[]`, so a client -keeps the list it already saw. Authors are not detail values, so the admin rule above leaves them in. +keeps the list it already saw. A custom `AuditStore` that doesn't implement `listTimeline` simply doesn't get this route mounted. One that implements `listTimeline` without `listTimelineUsers` serves the rows with no diff --git a/packages/agent/test/routes/access/audit-trail-timeline.test.ts b/packages/agent/test/routes/access/audit-trail-timeline.test.ts index c7d394d2e1..b849260bae 100644 --- a/packages/agent/test/routes/access/audit-trail-timeline.test.ts +++ b/packages/agent/test/routes/access/audit-trail-timeline.test.ts @@ -199,17 +199,6 @@ describe('AuditTrailTimelineRoute', () => { expect((context.response.body as { meta: object }).meta).not.toHaveProperty('availableUsers'); }); - - test('still names the authors to a non-admin, since they are not detail values', async () => { - const { route } = setup([row(1)], [jane]); - const context = contextWith({}, 'user'); - - await route.handleTimeline(context); - - expect( - (context.response.body as { meta: { availableUsers: unknown[] } }).meta.availableUsers, - ).toEqual([jane]); - }); }); describe('cursor', () => {