feat(audit_trail): record and expose record history in the agent, gated on an audit database - #320
Open
bexchauveto wants to merge 23 commits into
Open
feat(audit_trail): record and expose record history in the agent, gated on an audit database#320bexchauveto wants to merge 23 commits into
bexchauveto wants to merge 23 commits into
Conversation
|
Qlty doesn't post analysis or coverage comments for contributors without a seat. An authorized user can grant @bexchauveto a seat from this pull request's page in Qlty. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
Introduce the forest_admin_audit_trail package: a datasource-agnostic plugin that captures who changed what (before/after diff) for every change Forest performs through its data layer, with pluggable storage (in-memory, log, SQL). Supporting agent/rails wiring: - correlation id generated per request, propagated to the caller as request_id and echoed back via response header (CorrelationIdMiddleware) - record-history route (/_audit-trail/:collection/:id) reading from a configurable store - register the gem in the semantic-release pipeline (.releaserc.js) and exclude its version.rb from rubocop, matching the other packages Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Matches the other packages, which disable MFA and are excluded from the cop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add GET /_audit-trail/correlation/:correlation_key (one key) and the batch GET/POST /_audit-trail/correlations (correlationKeys list, POST body to dodge URL limits), both scoped to a record via collection/recordId params, sharing the per-record auth and store gate. Back them with list_by_correlation / list_by_correlations on the stores (SQL + in-memory + log no-op). Register the correlation source before audit_trail so /_audit-trail/correlation matches it instead of the per-record /_audit-trail/:collection_name/:id (Rails matches in definition order). Mirror the Node README: Rails configuration process plus docs for the record-history, correlation and batch correlation routes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Multiple SqlStore instances mutated the shared Sql::AuditLog.table_name, so stores against different Postgres schemas clobbered each other. Make AuditLog an abstract template and build a per-instance concrete subclass bound to the store's own qualified table. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Without the Postgres advisory lock, two instances booting at once can both see a migration as pending and both run its create_table/add_index, crashing the loser with "already exists". Add if_not_exists: true to the table and index DDL. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
An empty-string schema made schema? true, producing invalid identifiers like ".audit_migrations". Use present? so nil, "" and whitespace all mean no schema qualification. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…atabase The audit trail is no longer a separate gem to install and wire up: capture, storage and the record-history routes now live in forest_admin_agent and turn on as soon as `config.audit_trail[:database]` is set. - the agent factory builds the store from the configured database and installs the capture layer through the customizer, so `reload!` replays it - drop LogStore and InMemoryStore: with no database the feature stays off, and the specs cover the store through SQLite - AuditRecord becomes a Struct, and the capture hooks call their own methods instead of going through `send` - ActiveRecord is required lazily, so agents without an audit database never load it Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bexchauveto
force-pushed
the
feat/audit-trail-plugin
branch
from
August 10, 2026 14:00
5af078c to
9fcfeda
Compare
23 new issues
|
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
| add_delete_hooks(collection_customizer, columns, primary_keys, name, projection) | ||
| end | ||
|
|
||
| def add_create_hook(collection_customizer, columns, primary_keys, name) |
| end | ||
| end | ||
|
|
||
| def add_update_hooks(collection_customizer, columns, primary_keys, name, projection) |
| end | ||
| end | ||
|
|
||
| def add_delete_hooks(collection_customizer, columns, primary_keys, name, projection) |
| Thread.current[:forest_audit_trail_snapshots] ||= {}.compare_by_identity | ||
| end | ||
|
|
||
| def emit(caller, operation, collection, record_id, previous_values, new_values) |
| "Invalid date: \"#{raw}\" (expected YYYY-MM-DD or YYYY-MM-DDTHH:mm)" | ||
| end | ||
|
|
||
| instant.utc.iso8601(3) |
| else | ||
| base | ||
| end | ||
| end |
| request_id: nil, | ||
| project: nil, | ||
| environment: nil, | ||
| **_extra_args |
…e without activerecord Rails eager loads every Zeitwerk loader, so the audit `sql/` files were pulled in — and with them `require "active_record"` — in agents that never configure an audit database (the mongoid package's CI job boots such an app). They are excluded from eager loading now. Review findings: - the history routes only checked `can?(:read, collection)`, so a role restricted to a subset of records could read the history of any of them; both routes now intersect the record with the permission scope, like Show - migration bookkeeping keyed the target table, otherwise a store configured with another `table_name` read someone else's migrations as done and never created its table - snapshots pair through a bounded LIFO stack instead of the filter object: the after-context carries the caller's original filter, so a customization replacing it silently dropped the audit record and stranded the snapshot - the effective patch is snapshotted in the before hook, so a replaced patch is recorded as written rather than as sent - a hash key holding nil is no longer diffed as equal to a missing key The reported `list` arity issue is a false positive: hook contexts hand out a RelaxedCollection, whose `list(filter, projection)` already binds the caller. The capture spec now uses a verifying double of it so the contract is asserted. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…afe page parsing Review follow-ups: - history stays readable once the record is gone: the scope check now refuses only a record that still exists outside the caller's scope, since inspecting what was deleted is much of the point of an audit trail - the correlation routes serialize like the per-record one (camelCase on top, column names untouched) instead of leaking snake_case keys; the shared behaviour of the audit routes lives in AuditTrailRoute - `?page=foo` no longer raises: a non-Hash `page` falls back to the documented defaults instead of blowing up in Hash#dig Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
bexchauveto
changed the title
…outes `GET /_audit-trail/correlations` has two path segments, so the earlier `/:collection_name/:id` route matched it first and answered "Collection '_audit-trail' not found". Both audit-trail sources now sit ahead of every route matching on `:collection_name`; everything registered before them has a literal first segment, so nothing can shadow them and they shadow nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The customizer has no Execute hook, so the invocation is recorded from the action route: one row per selected record, `operation` `action` (or `action_failed` when it raised), the action name in its own column and the submitted form values in `new_values`, redacted with the same config as columns. A global action or a select-all selection names no target without querying the whole selection, so those runs are recorded once, attached to no record. Recording is best-effort: a failing audit database logs instead of breaking the action. What an action *changes* is unchanged: writes going through the Forest data layer are audited by the existing hooks under the same correlation key, and a direct ORM write stays invisible — documented rather than worked around. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…y have it The Forest activity logs record which action ran, and `correlation_key` joins them to the audit rows, so storing the name again buys nothing. Migration 003 goes with it: the action row keeps `operation` (`action` / `action_failed`), the submitted form values and the targeted record ids. Anyone who ran the intermediate code keeps a stray nullable `action_name` column; nothing reads or writes it, and it can be dropped by hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| @redact = redact || {} | ||
| end | ||
|
|
||
| def record(caller:, collection:, form_values:, record_ids:, failed: false) |
|
|
||
| # A failed run is worth recording too: "who tried to run this" is usually the interesting part. | ||
| # The audit row is best-effort — a broken audit database must not fail the action itself. | ||
| def execute_and_audit(context, args, data, filter) |
| raise | ||
| end | ||
|
|
||
| def audit_action(context, args, data, failed: false) |
… gaps Recording a change raised through the after hook when the audit store was unavailable, so a write that had already succeeded came back as an error the client could retry into a duplicate. Auditing is best-effort now, in one place (`Recording#audit_safely`): the failure is logged and the row dropped, for the change capture, the snapshot read of a before hook (which would otherwise block the write) and the action capture alike. A failed snapshot read pushes an empty snapshot rather than nothing, so the after hook still pops its own entry instead of an unrelated one. Coverage of the new code, following the qlty report: - the action audit path, previously untested (72% diff coverage on actions.rb) - the Postgres-only migrator branches — advisory lock, schema creation and its concurrent-create rescue — through a fake adapter, no server needed - the wall-clock datetime filters, documented but never exercised, including an out-of-range instant - the correlation routes' unknown-collection 404 and its re-raise - three route registrations now dispatch through their closure instead of calling the handler, so the wiring is covered by the tests that were already there Every audit-trail file is at 100% except the `activerecord` LoadError guard, which needs a bundle without ActiveRecord to reach. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| ) | ||
| ) | ||
| end | ||
| end |
…us_values Hooks bracket the write as separate calls and the data layer exposes no lock or transaction primitive — deliberately, since it spans ActiveRecord, Mongoid, HTTP APIs and more. Two writes racing on one record therefore snapshot the same state, and the second records a previous_values that was already overwritten. Nothing to fix inside the capture layer: reading the snapshot in the write's transaction is not something a hook can do, and after the write the before-state is gone. Documented in the code and in AUDIT_TRAIL.md, next to the ORM-write limitation it belongs with. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Hooks#execute_after` stops at the first exception, and the capture layer is installed last, so any customization raising in its own after hook dropped the audit row of a write that had already happened — and stranded its snapshot. `add_hook` takes a `prepend` flag now (optional, defaults to appending as before), and the capture layer prepends its after hooks so nothing can preempt them. Its before hooks stay appended, so the snapshot still reads the filter and patch every other customization has had its say on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| # end | ||
| def add_hook(position, type, &handler) | ||
| push_customization { @stack.hook.get_collection(@name).add_hook(position, type, handler) } | ||
| def add_hook(position, type, prepend: false, &handler) |
|
|
||
| def add_hook(position, type, hook) | ||
| @hooks[type].add_handler(position, hook) | ||
| def add_hook(position, type, hook, prepend: false) |
Parity with the Node agent, plus the two gaps found on the way. - `GET /_audit-trail/:collection/:id/state?timestamp=` rebuilds a record by undoing every entry recorded strictly after the instant (an entry stamped exactly at it belongs to that state). A create means the record did not exist then; a delete restores the row it recorded and the walk carries on into an earlier life of the same id. - The diff now leaves a key out of the side where it does not exist instead of writing nil, so "key added" and "key holding nil" stay tellable apart — `Diff.revert` needs that, and a round-trip property spec pins it. No sentinel value reaches the database. - `fields` filter on the history route, with the JSON key test per adapter (Postgres / SQLite / MySQL, raising on anything else) in its own FieldFilter class. Field names are quoted as whole keys, so a name holding a dot is not read as a path, and both sides of the diff are searched since an added key exists on the new side only. - The correlation-id middleware goes ahead of ShowExceptions: a Rack middleware can only add a header to a response it sees returned, so sitting under the exception handlers meant the 500s they build never carried the id. - The schema-already-exists rescue checks SQLSTATE (42P06 / 23505) where the adapter exposes one, and RecordNotUnique by class, instead of matching "already exists" anywhere in a message. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| next_values[key] = sub[:next] unless sub[:next].equal?(ABSENT) | ||
| end | ||
|
|
||
| { previous: previous, next: next_values } |
| # created the schema between our IF NOT EXISTS check and the create itself. | ||
| nil | ||
| rescue ActiveRecord::StatementInvalid => e | ||
| raise unless duplicate_schema?(e) |
| model.create!(to_row(record)) | ||
| end | ||
|
|
||
| def list_by_record(collection:, record_id:, skip: 0, limit: nil, user_ids: nil, |
| relation.map { |row| from_row(row) } | ||
| end | ||
|
|
||
| def count_by_record(collection:, record_id:, user_ids: nil, start_timestamp: nil, |
|
|
||
| private | ||
|
|
||
| def scope(collection, record_id, user_ids, start_timestamp, end_timestamp, fields = nil) |
…cises The specs run on SQLite, so the Postgres and MySQL field filters were never executed. FieldFilter only builds a condition string, so a fake connection is enough to pin all three — including that neither emits a `?` (a bind placeholder for ActiveRecord) nor `json_extract` (which cannot tell a key holding null from a missing one). Also covers the SQLSTATE read failing, and dispatches the state route through its registered closure so the wiring is exercised like the other three. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e timezone Two review findings, both hidden by specs that asserted an encoding by hand instead of the one the diff actually emits. - `diff_object_arrays` recorded `previous[index] = nil` for an index only the new side reaches, so reverting an append left a nil hole instead of shortening the array. Indexes now follow the same rule as hash keys — left out of the side that does not reach them — and the round-trip spec carries length changes so an append and a drop are both covered. - `parse_state_timestamp` handed `2026-01-02T08:30:15` to Time.iso8601, which parses it happily in the server's timezone and ignores the request's. Wall-clock values (with or without seconds, and bare days) go through the request timezone; Time.iso8601 is left for values carrying an offset or Z. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| next_values[index] = sub[:next] if index < after.length | ||
| end | ||
|
|
||
| { previous: previous, next: next_values } |
`handle_state` authorized the record with a scoped query and then read its columns with an unscoped one, so the row it returned was not the row the check covered. Authorization and read are now the same query: `scoped_record` returns the record matched against the caller's scope (nil when it is simply gone, 404 when it exists outside that scope), and `assert_record_in_scope` is that same call with its result discarded. Nothing reads the row around the scope any more, and /state does one query instead of two. The spec that was meant to cover this passed for the wrong reason — it consumed the primary-key stub as the record, so the full read was never exercised. It now asserts the scope is in the filter, that every column is projected and that a single query happens, plus the two branches around it: a record living outside the scope, and a deleted one rebuilt from its history. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| # | ||
| # Authorizing and reading are the same query on purpose: a scoped check followed by an unscoped read | ||
| # would hand back a row the check never covered, the moment the two drifted apart. | ||
| def scoped_record(context, collection, packed_id, projection = nil) |
| Projection.new(ForestAdminDatasourceToolkit::Utils::Schema.primary_keys(collection)) | ||
| end | ||
|
|
||
| def first_record(context, collection, condition_tree, projection) |
…e agent
Node's handleStateAt answers with `{ data }` and nothing else, so the timestamp
and reverted-entry count go: they were ours to invent and nothing reads them.
`timestamp` and `entries` are still needed to query and rebuild, only the meta
hash is gone.
The spec now pins the whole payload rather than asserting on the two keys, so a
stray meta key fails instead of passing unnoticed, and AUDIT_TRAIL.md drops the
note about the shape being unconfirmed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ion stack An insert_before recorded on the engine's own middleware proxy does not see ShowExceptions and raises when the stack is applied — too late for the rescue here, which only guards the recording. It goes to Rails.application's stack instead, where the exception handlers actually live. The spec followed the engine's proxy, so it was updated to the application's, and gained the case where there is no application to reach at all (the rescue covers it by appending to the engine stack, as before). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An action reporting failure through `result_builder.error` never raises, so the
run was recorded as `action`. It answers with `{ type: 'Error', ... }`, which is
as failed as a raised one — same rule as the Node agent's `result.type === 'Error'`
— and the result is still returned to the caller untouched.
The result-type read is guarded, since it happens before ActionResult.parse and an
action need not answer with a hash at all.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds a built-in audit trail to
forest_admin_agent: every create / update / delete Forest performsthrough its data layer is recorded with who did it, when, and the minimal before/after diff; smart-action
runs are recorded too; and five routes expose that history — including a state reconstruction that
rebuilds a record as it stood at any past instant.
It is off unless an audit database is configured — no route registered, no ActiveRecord loaded, no hook
installed:
Why
This started as a separate plugin gem (
forest_admin_audit_trail). Capturing changes needs the customizerhooks, the caller's per-request id and the agent's route stack — agent internals the plugin had to reach
into from outside, and users had to wire the same store instance into two different places by hand (the
agent option and
agent.use(Plugin, store:)) or silently get a half-working feature. Folding it in turnsthat into one config key, and the capture layer is installed by the agent factory so a
reload!replays itlike any other customization.
ActiveRecord stays optional: the storage files are autoloaded on first use only, and excluded from Zeitwerk
eager loading (Rails eager loads every gem loader), so agents without an audit database never load it.
Outside Rails, add
gem 'activerecord'plus your adapter.Contents
Capture & storage —
packages/forest_admin_agent/lib/forest_admin_agent/audit_trail/Capture— instruments every collection through the customizer hooks; datasource-agnostic, so it behavesthe same on ActiveRecord, Mongoid, etc. Audits writable columns only, and registers its after hooks ahead
of everyone else's since
execute_afterstops at the first exception.ActionCapture— records smart-action runs:operationaction/action_failed, the submitted formvalues, one row per selected record.
Diff— structural before/after diff (only changed leaves are stored) andrevert, its inverse. A keyabsent on one side is left out of that side rather than written as
null, so "key added" and "keyholding null" stay tellable apart without any sentinel value reaching the database — which is what lets a
revert remove a key instead of resurrecting it as null.
RecordState— walks the history backwards to rebuild a record at an instant.Store+Sql::{AuditConnectionBase,AuditLog,Migrator,FieldFilter}— ActiveRecord-backed storage on itsown connection pool, creating/evolving
forest.audit_logsthrough versioned migrations (advisory-lockedon Postgres, keyed per target table, run lazily on first use), and reading the history back.
Recording— the shared policy: correlation key, redaction, and auditing never breaks the request (afailure is logged and the row dropped — by the time we record, the write has happened).
Routes (all under
/forest/_audit-trail, all requiringcan?(:read, collection)and the caller'spermission scope on the target record)
GET /_audit-trail/{collection}/{id}— paginated record history, filterable byuserIds,startDate/endDate(wall-clock in the request timezone) andfields.GET /_audit-trail/{collection}/{id}/state?timestamp=— the record as it stood then, rebuilt by undoingevery entry recorded strictly after that instant.
dataisnullwhen it did not exist yet.GET /_audit-trail/correlation/{key},GET|POST /_audit-trail/correlations— history grouped by theper-request correlation key.
Per-request correlation
CorrelationId+CorrelationIdMiddleware— one id per request, exposed on the caller asrequest_id,echoed back in the
X-Forest-Correlation-Idheader (CORS-exposed), so every change made in one requestshares a key — and joins to the Forest activity log, which is where the action's name lives. Mounted
ahead of
ShowExceptions: a Rack middleware can only add a header to a response it sees returned, sounder the exception handlers the 500s they build never carried the id.
Wiring & docs
AgentFactorybuilds the store from the config and installs the capture layer onbuild;ForestAdminRailsandForestAdminRpcAgentexpose theaudit_trailsetting.Hooks#add_handler(prepend:)flag in the customizer (optional, appends as before).packages/forest_admin_agent/AUDIT_TRAIL.mddocuments configuration, routes, stored columns, migrationsand the limitations below.
Field filtering, per adapter
fieldskeeps only entries whose diff touched one of the given names, in SQL so paging and counts staycorrect. Both JSON columns are searched (a field the change added exists in
newValuesonly), and a name isalways a whole key —
address.cityis quoted, never read as a traversal. Postgres usesjsonb_object_keysrather than
?|(which ActiveRecord reads as a bind placeholder), SQLite usesjson_typerather thanjson_extract(a key holding JSON null extracts as SQL NULL, indistinguishable from missing), MySQL /MariaDB use
JSON_CONTAINS_PATH. Any other adapter raises instead of silently returning everything. Thesuite runs on SQLite, so the Postgres and MySQL strings are pinned by their own specs.
Limitations, stated on purpose
Customer.find(id).update!(...)is invisible to the agent; only its invocation row exists. Worth auditingexisting actions before promising customers full coverage.
previousValues. Hooks bracket the write as separate calls and thedata layer exposes no lock (deliberately — it spans ActiveRecord, Mongoid, HTTP APIs), so two writes
racing on one record snapshot the same state.
newValuesis always exact and no row is lost. Exactbefore-images under concurrency need triggers or CDC.
recorded, so they cannot be restored, and a delete's snapshot holds writable columns only.
the caller's scope. Once no record exists there is nothing to evaluate a scope against, so that history is
readable by anyone with collection read permission — deliberate, the
deleteevent is the point.the per-record route. Nothing reads those endpoints yet (no reference on the frontend or
agent-nodejsdefault branches), so consistency was the tie-breaker — worth confirming when the Historic tab lands.
/statenow matches Node'shandleStateAt:{ data }and nothing else.redactmasks values but still records the change, and applies to smart-action form values too, keyed bycollection.
Notes
No new package, so nothing was added to
.releaserc.jsor the CI matrices; the audit trail ships withforest_admin_agent. Out of scope but spotted on the way:Utils::QueryStringParser#parse_paginationdigsinto
params[:page]assuming a Hash, so?page=fooraises for every list/count route — left alone since itraises
BadRequestErrorrather than defaulting, so fixing it is a behaviour change of its own.🤖 Generated with Claude Code
Note
Add audit trail feature to record and expose collection record history gated on an audit database
AuditTrail::Captureto instrument collections via hooks, computing minimal diffs per record and supporting per-collection field redaction;AuditTrail::Storehandles lazy schema migration and stable paginated queries./_audit-trail/:collection/:id(paginated history with user/date/field filters) and/_audit-trail/:collection/:id/state(record state reconstructed at a past timestamp usingAuditTrail::RecordState)./_audit-trail/correlation/:key,/_audit-trail/correlations) and aCorrelationIdMiddlewarethat generates a per-request UUID and emits it as thex-forest-correlation-idresponse header.audit_trail: { database: ... }in agent options; no store means no routes or capture hooks are installed.listqueries (snapshots) for updates and deletes, increasing DB load per write.Macroscope summarized 9ac9ffc.