feat(audit_trail): record and expose record history in the agent, gated on an audit database - #320
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}
5af078c to
9fcfeda
Compare
26 new issues
|
![qltysh[bot]](https://avatars.githubusercontent.com/in/890766?s=60&v=4){kind=small}
| "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 |
bexchauveto
changed the title
| # 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) |
| 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) |
| next_values[index] = sub[:next] if index < after.length | ||
| end | ||
|
|
||
| { previous: previous, next: next_values } |
| # | ||
| # 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) |
PMerlet
left a comment
PMerlet
left a comment
There was a problem hiding this comment.
Adversarial review pass, read together with agent-nodejs#1686 and the front PR. Several comments below are alignment points with the Node agent rather than problems visible from this diff alone — the two agents write different data for the same event today, and one client renders both.
For context, the architecture we settled on while reviewing:
- Capability, not feature flag — the agent advertises the audit-trail capability; the History tab exists only when the customer's agent has the trail configured.
- Two tabs, two sources, no cross-hydration — Activity keeps reading activity logs, History reads the audit trail and nothing else.
- The audit row is self-sufficient — it must carry the actor identity and the action name, because the activity log is written client-side after the commit, in one unretried attempt, with failures swallowed. It is structurally less complete than the trail.
- Failure policy, identical in both agents: pending before, confirm after — the row is written in a
pendingstate before the write and confirmed after. A failed pre-write insert refuses the operation without having touched the data. No compensating write, ever. - Pagination and filters belong to
GET /_audit-trail/{collection}/{id}— the per-record route this PR already implements, which the front should call instead of/correlations.
The Diff design here is the better of the two: leaving an absent key out of that side rather than writing a sentinel is what Node should align on — its \u0000 sentinel is unstorable in a PostgreSQL jsonb cast and reaches the front as a raw control character. Flagged there.
Six things I'd want settled before this lands: the failure policy, record_id width, the ids resolved outside the safety net, URL sanitising on action results, narrowing action ids to the caller's scope, and the newValues/record_id divergences with Node.
The code is careful and the comments anticipate most of the obvious objections — none of what follows is about sloppiness.
| t.datetime :timestamp, null: false | ||
| t.string :operation, null: false | ||
| t.string :collection, null: false | ||
| t.string :record_id, null: false |
There was a problem hiding this comment.
Blocker — record_id is VARCHAR(255), and a packed composite id can exceed it.
agent-nodejs#1686 shipped a third migration for exactly this, with the failure mode written on it: "record_id was VARCHAR(255): a packed composite id can exceed that, silently failing every write for the affected record. Widened to TEXT; the index is rebuilt with a length prefix so it stays valid on MySQL/MariaDB, which cannot index an unbounded TEXT column directly."
Worth settling now rather than later: the list is append-only by design, so every database that has already run 001 needs the widening as its own entry regardless — and the pending/confirmed status column (see the audit_safely comment) adds another one on top.
Node also documents something this list has no equivalent for: on dialects that implement changeColumn as a table rebuild (sqlite), every existing index is dropped along with the old table, so 002's indexes need re-creating afterwards.
There was a problem hiding this comment.
Fixed, though not as an added migration — worth flagging since it diverges from what you suggested.
Nothing has been released, so rather than appending a widening entry we squashed the whole list into the single create it would have produced (793bc22). record_id is t.text and nullable from the start, and the index carries length: 255 on MySQL/MariaDB because the column is TEXT from the outset. The append-only rule you invoke assumes a shipped 001; there isn't one, so there is no database out there whose history we would be rewriting.
That also disposes of the sqlite point: the table-rebuild hazard belongs to change_column, and there is no longer a widening step to rebuild anything. Had we shipped first, the entry would have had to drop the index, widen, then re-create all three — which is what the intermediate commit did before the squash, if you want to see it: 9185f92.
The status column you anticipated is in the same create (null: false, no default — every write sets it, so a row arriving without one is a NOT NULL violation rather than a row quietly claiming to be done), along with the denormalised identity columns and action_name.
Two things came with it: the tracker moved to audit_logs_migration, named after the table it describes, so two stores with different table_names keep separate histories — which is what an earlier per-table key prefix was working around. And a spec stores a 40-part composite id to pin the failure mode you named.
| # already happened, so raising would report a failure for an operation that succeeded (and invite a | ||
| # retry that duplicates it); a snapshot read failing must not block the write either. Losing the row | ||
| # is the lesser evil, so it is logged and dropped. | ||
| def audit_safely |
There was a problem hiding this comment.
Blocker — the failure policy needs to change, in both agents.
This is the honest version of fail-open, and the reasoning is sound as far as it goes: by the time you record, the write has happened, so raising would report a failure for an operation that succeeded. agent-nodejs#1686 does the opposite on its CRUD path — it awaits the sink inside the After hook with nothing catching it, so a broken store 500s the request after the commit. Two agents, two opposite behaviours, neither of them the policy.
What we settled on is pending before, confirm after: write the row in a pending state before the write, confirm it after. A failed pre-write insert then refuses the operation without having touched the data, so there is nothing to repair and no compensating write is ever needed — which matters, because a post-hoc "revert" is irrecoverable on delete (a read-only primary key is absent from previous_values, which holds writable columns only), can clobber a concurrent legitimate write, and can fail itself.
The invariant this buys is no unaudited write, not every row holds exact after-values: if the confirm fails you're left with a pending row holding the intent rather than the persisted state. That residue is detectable, which is the whole point, and a pending row is legitimate audit information — an attempt.
There is a precedent for the pattern in the Node monorepo: packages/mcp-server does createPendingActivityLog then updateActivityLogStatus.
Small thing in this file while I'm here: correlation_key_for falls back to SecureRandom.uuid when the caller has no request_id. A fabricated key groups the row into a request of its own, indistinguishable from a real single-row request — nil is more truthful than an invented join value. Same for CorrelationId.current's ||=.
There was a problem hiding this comment.
Implemented as described, in 9185f92 — pending before, confirm after, one code path in both modes so status always means the same thing. config.audit_trail[:critical] (default false) decides what a failed pending insert costs: under true the operation is refused with nothing written, under false the row is logged and dropped as before.
The invariant is written down as you framed it, in AUDIT_TRAIL.md and on the class: no unaudited write, not every row holding exact after-values. A row left pending is an attempt, and the residue is the point. Everything after the pending insert stays best-effort in both modes, since by then the write has happened.
Three places where following that through changed more than the hook bodies:
- A selection wider than the 500-record cap used to be audited in part while the write touched every match — the same hole in a different guise. Under
critical: truethat is refused now; underfalseit truncates and logsN audited, M skipped(1d95edb). - A record the agent cannot read back after the write keeps its row pending, rather than being confirmed from the patch. Same reasoning as yours: the patch is intent, not evidence.
- The state reconstruction skips pending rows. Undoing a change that may never have happened would hand back a state the record was never in. The history reads keep them, since that is where the evidence belongs.
On the fabricated correlation key: agreed, and fixed in 1d95edb — correlation_key_for returns nil when the caller carries none, so a write outside any request reads as belonging to no request instead of to a single-row one of its own.
I left CorrelationId.current's ||= alone, and want to be explicit about why in case you disagree. It is the generator: CallerParser calls it inside a request the middleware has already reset, and current? (non-generating) is what the middleware reads for the response header. Making current non-generating would mean nothing ever generates the id. The real hazard there is a host that never mounts the middleware — no reset, so a pooled thread hands its previous request's key to the next one, which is worse than fabrication — so AUDIT_TRAIL.md now says a non-Rails host has to mount it.
| @model | ||
| end | ||
|
|
||
| def ensure_ready |
There was a problem hiding this comment.
Lazy connection is no longer viable under the failure policy.
This connects on first append and runs the migrations then, and a failure is swallowed upstream by audit_safely — so a bad database: config today means an audit trail that silently records nothing, forever, while the agent looks healthy. agent-nodejs#1686 made the opposite choice: the connection and migrations run during agent.start(), so a bad connection string or a broken migration fails fast at boot rather than on the first request.
With pending-before-write, lazy gets worse rather than merely different: a store discovered broken at first use would refuse every write instead of refusing to boot. Validating at boot is the behaviour that makes the policy legible to whoever deploys it.
Also in this method: AuditConnectionBase.establish_connection is class-level, so two stores configured with different database: values clobber each other's pool. The comment on build_model covers the table_name collision but not this one.
There was a problem hiding this comment.
Both fixed in 9185f92.
The store connects and migrates in AgentFactory#build_audit_trail_store, at setup — a database the agent cannot reach now stops it starting instead of leaving it looking healthy while recording nothing. Your point about the interaction with the policy is what made it non-optional: under critical: true, a store discovered broken at first use would refuse every write rather than refuse to boot.
The class-level pool is guarded rather than made per-store: AuditConnectionBase.connect_to remembers the config it established and raises if asked for a different one — "One agent, one audit database." Two stores on the same database with different table_names stay supported (each gets its own model and its own migration tracker), which is the case that had a legitimate use; two different databases is a configuration mistake, and now says so at boot instead of silently sharing a pool. Specs cover both.
| # | ||
| # An empty snapshot on failure rather than no snapshot at all: the after hook pops unconditionally, | ||
| # so skipping the push would pair it with an unrelated entry. | ||
| def snapshot(context, projection) |
There was a problem hiding this comment.
Blocker — the snapshot lists every matched record, unbounded.
context.collection.list(context.filter, projection) has no limit, and the result is held on the thread-local stack until the matching after hook. A "delete all" on a large collection materialises every matched row in the agent process before the write runs.
What makes this worth a blocker rather than a note: it silently changes the cost of a write path customers already use, behind a config key they may well enable in production without re-testing bulk operations. And under the pending-then-confirm policy the same volume gets written twice. Same pattern and same comment on agent-nodejs#1686.
Suggestion: batch the inserts (insert_all), bound the snapshot, and make truncation explicit and logged — "N records audited, M skipped" beats an OOM, and beats silence.
On MAX_SNAPSHOTS and the LIFO stack: the pairing is sound for one operation on one thread, and I like that it doesn't depend on the filter object reaching both hooks. Worth noting the residue it accepts — a write raising between the hooks strands its entry, and the cap drops the oldest, so stranded entries sit on the stack for up to 15 subsequent operations. Node correlates by patch content to avoid handing a later operation someone else's snapshot; both approaches have a hole, and pending-then-confirm gives you a monotonic token that closes it properly for free.
There was a problem hiding this comment.
Fixed in 9185f92, and tightened again in 1d95edb once you and Macroscope pushed on it from opposite sides.
The snapshot is capped at AuditTrail::MAX_RECORDS_PER_OPERATION (500), read with limit: cap + 1 so truncation is detectable, inserted with insert_all, and truncation logs exactly what you asked for: N records audited, M skipped at Warn, with the real M from a count query issued only when truncated.
One thing the cap alone got wrong, which 1d95edb fixes: under critical: true, auditing 500 rows while the write touches every match is the invariant that mode exists for. Over the cap it now refuses the operation, in the before hook, so nothing is written. The snapshot read went through the same gate for the same reason — knowing what an operation is about to touch is part of being able to record it.
On the LIFO residue: I have not closed it, and I want to be straight about why rather than claim the token did it. Pairing is still positional, because the after hook has no token of its own to look the snapshot up by — the pending row ids live in the stack entry, so they cannot also be what finds it. What did change is that the residue stopped being silent: a stranded entry now corresponds to a pending row sitting in the audit table, which is queryable evidence that an operation started and never settled, where before it was an invisible entry on a stack. If you have a way to get a token to the after hook that I have not seen, I would take it.
| push_snapshot(records: snapshot(context, projection), patch: context.patch) | ||
| end | ||
|
|
||
| collection_customizer.add_hook('After', 'Update', prepend: true) do |context| |
There was a problem hiding this comment.
Two divergences with the Node agent, both changing what the record asserts.
new_values source. Diff.changed_values(record, snapshot[:patch], columns) diffs against the patch. agent-nodejs#1686 re-reads the persisted record after the write and diffs against that, deliberately: "Diffs against the record actually persisted by the write (after), not the requested patch, so the log reflects normalization/coercion/side effects applied by the datasource or a decorator." So for the same edit through a coercing decorator, the two agents store different "after" values. It also decides what /state can faithfully reconstruct.
record_id when the update changes a primary key. record_id(record, primary_keys) uses the before snapshot, so the row is filed under the old id. Node packs from the post-update values with the reasoning spelled out: "if the update changed a writable primary key, the entry is filed under the record's new id, matching what later history lookups use." Since History queries by the record's current id, a row filed under the old one is unreachable from that record's own timeline here.
Both are worth aligning explicitly rather than leaving to whichever agent a customer happens to run — the front renders both.
On prepend: true — this is load-bearing here, since execute_after stops at the first exception and the hooks share one decorator with user hooks. Worth a comment saying so, because on the Node side the same option turns out to be unnecessary (audit hooks live on a separate inner decorator that already runs first), and the asymmetry will look like an oversight otherwise.
There was a problem hiding this comment.
All three aligned, in 9185f92.
new_values is diffed against the record as re-read after the write, so normalisation and decorator side effects are what gets recorded. The re-read is one query for the whole batch, keyed by each record's own id.
record_id is packed from those post-update values, so an update that moves a writable primary key files the row under the id later history lookups use. A spec covers it with a writable primary key — worth noting it only bites when the key is writable, since otherwise the patch cannot touch it.
One consequence of doing the re-read, since Macroscope caught it after you: when the lookup comes back empty — read failed, or the datasource normalised the key past what we asked for — the row is left pending rather than confirmed from the patch. Confirming from intent would assert values that may never have been written.
prepend: true now says why on the method: execute_after stops at the first exception and the hooks share one decorator with user hooks, so being last would mean losing the record of a write that already happened. Thanks for flagging the asymmetry with Node — a reader comparing the two would otherwise read it as an oversight.
| # business in an audit table and the other two routinely hold credentials, and an allowlist means a | ||
| # field added to a result later is not stored until someone decides it should be. `html` is left out | ||
| # too: operator-facing markup, sometimes large, and the message already says what happened. | ||
| RESULT_FIELDS = %i[type message name mime_type method url path].freeze |
There was a problem hiding this comment.
A Webhook or Redirect result's URL is persisted verbatim.
The allowlist rationale is right, and dropping html, the file contents and the webhook body/headers is the correct call. But url and path go through slice untouched, and agent-nodejs#1686 strips them for a reason worth borrowing: "either can carry credentials (userinfo) or a signed/one-time token (query string) that must not be written permanently to the audit database — only the origin and path are kept." Its sanitizeUrl also has a fallback branch for URLs the parser rejects, which is exactly where such a token would still sit.
That lands a live token permanently in the one table nobody is supposed to delete rows from.
Two more things on this class:
Key naming. mime_type here vs mimeType on Node. The front renders these keys verbatim (it only camelCases an entry's top-level keys), and its tests currently encode mime_type — so a Node agent shows a differently-labelled field for the same result. Cheap to align, worth writing the canonical shape down once somewhere both agents cite.
Contract addition. The action name should be recorded on action / action_failed rows. The comment defers it to the activity log, which was reasonable while the log was the timeline's spine — but the log is written client-side after the commit, in one unretried attempt whose failure is swallowed, so it can simply be absent. Once History reads the trail alone, a row whose log went missing renders as "Action — user 7" with the name nowhere. @action_name is in scope at the call site in routes/action/actions.rb. Same for the actor: user_id alone can't render a name, and denormalising it captures who acted then rather than who holds that id today.
There was a problem hiding this comment.
All four done, across 9185f92 and 5610850.
URLs. url and path go through sanitize_url now: userinfo, query and fragment removed, origin and path kept. The credentials come off textually before parsing, because URI#userinfo = nil is a silent no-op in Ruby — which cost me a failing spec to discover. The parser-rejects branch is covered too (https://user:pass@pay test/refund?token=abc), since that is precisely where a token would otherwise survive.
Key naming. Answer keys are camelCased on write, so mimeType. Since the front's tests currently encode mime_type, that test flips — the canonical shape is written down in AUDIT_TRAIL.md and in the API hand-off the front is getting.
Action name. Recorded on the row again. Worth recording how it went: it was dropped in 5610850 on the reasoning that the activity log already holds it, and your argument for putting it back — the log is written client-side, post-commit, unretried, and its failure is swallowed, so History reading the trail alone would render "Action — user 7" with the name nowhere — is what reversed that. It is now a column set from @action_name at the call site.
Actor. Denormalised: user_first_name, user_last_name, user_email alongside user_id, read from the caller at write time, so a row says who acted then rather than who holds that id today. Read defensively, since a caller built by another code path need not carry a full identity and a NoMethodError there would refuse the write under critical: true. meta.availableUsers on the history route is built from those same columns.
| result.is_a?(Hash) && result[:type] == 'Error' | ||
| end | ||
|
|
||
| def audit_action(context, args, data, result: nil, failed: false) |
There was a problem hiding this comment.
Blocker — the audited ids are resolved even when the audit trail is off, and outside the safety net.
record_ids: audited_record_ids(args, context) is an argument, so it is evaluated before ActionCapture#record reaches its return unless @store guard, and outside audit_safely. Two consequences:
- Every smart-action execution parses the selection a second time, for every user, whether or not an audit database is configured.
- If that parse raises,
execute_and_audit'srescue StandardErrorcatches it, callsaudit_actionagain, raises again — and the request 500s, discarding the result of an action that already ran successfully.
agent-nodejs#1686 guards both: resolveAuditedRecordIds returns [] immediately when auditTrail is null, and wraps the lookup in its own try/catch with the same reasoning.
Suggestion: return unless ForestAdminAgent::AuditTrail.store at the top of this method, and move audited_record_ids inside the protected block.
There was a problem hiding this comment.
Fixed in 9185f92, and both consequences you list are gone.
audit_pending starts with return [] unless ForestAdminAgent::AuditTrail.store, so an agent without an audit database never parses the selection at all. And the selection now happens inside AuditTrail.gate, together with the pending insert, so a failure there cannot 500 an action: under critical: false it is logged and dropped, under critical: true it refuses the run — which is sound, because the gate runs before execute, so nothing has happened yet.
The 500 you described also went away structurally: with pending-before/confirm-after there is no longer a post-hoc audit_action in the rescue path that could raise a second time. The rescue only confirms rows that already exist, and confirming is best-effort.
| # Packed ids, the form the audit store keys on. A global action targets nothing, and a select-all | ||
| # selection only tells us which ids were *excluded*: naming the targets would mean querying the | ||
| # whole selection, so those runs are recorded once, attached to no record. | ||
| def audited_record_ids(args, context) |
There was a problem hiding this comment.
The ids aren't narrowed to the caller's authorized subset, so the trail can assert an action touched records it didn't.
This packs whatever ids the client sent, minus are_excluded. agent-nodejs#1686 re-lists through filterForCaller — the same filter execute() itself received — with the reasoning spelled out: "An explicit selection is narrowed down to the caller's own authorized subset … so an id excluded by scope doesn't get an entry for an action that never touched it."
In a compliance record a false positive is worse than a missing row: it states that an operator acted on a record when the scope prevented it. Worth narrowing through the caller's filter here too.
There was a problem hiding this comment.
Fixed in 9185f92 — narrowed through the caller's own filter, as on the Node side.
audited_record_ids no longer touches the request's ids. It lists through filter_for_caller — the same filter execute() receives — with a primary-key projection, and packs what comes back. So an id the scope excludes gets no row, and the false positive you describe (the trail asserting an operator acted on a record the scope prevented) cannot happen.
Two details that came with it. The listing is capped at 500 like every other audited read, and a wider selection is recorded as one row attached to no record rather than a partial list of targets that would read as complete. And a global action still queries nothing at all, since it targets no record by definition.
| # | ||
| # 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) |
There was a problem hiding this comment.
Worth an explicit product sign-off rather than only a code comment.
Authorising and reading in one query is the right instinct, and the rule — a scope is enforced only while the record still exists — is reasoned carefully here and in the PR description. The adversarial reading: record ids are guessable, so this is an enumerable endpoint returning the full prior contents of deleted records (the delete row holds every writable column) to users who were never allowed to see them while they existed. For a role scoped to one team's records, deleting a record widens who can read it.
More salient now that History becomes the primary tab for customers who enable the trail. If the rule stands, one middle option: withhold the delete row's previous_values from out-of-scope callers while still surfacing that a deletion happened, by whom and when — the part the comment argues is "much of the point".
Either way I'd want this in the ticket and the customer-facing docs, not only in a source comment. Identical rule and identical question on agent-nodejs#1686.
There was a problem hiding this comment.
Not fixed, and deliberately so — this one is @bexchauveto's call and he has made it: history stays readable for a record that no longer exists, for audit purposes. So the rule you describe stands, including its consequence that deleting a record widens who can read what it contained.
What is now true, and was not when you wrote this: authorisation and the read are a single query (scoped_record), so a record that still exists outside the caller's scope is refused rather than being checked by one query and read by another. The rule applies only where there is no record left to evaluate a scope against.
Your middle option — surfacing that a deletion happened, by whom and when, while withholding the delete row's previous_values from out-of-scope callers — is the part I would want a decision on rather than an implementation from me, since it trades away exactly the content the current rule was chosen to preserve. Flagged to @bexchauveto with your framing.
Agreed on the process point regardless: this belongs in the ticket and the customer-facing docs, not only in a source comment. It is currently in AUDIT_TRAIL.md and the PR description, which is not the same thing as a customer knowing it.
| end | ||
|
|
||
| # Every column, the starting point of a state reconstruction. | ||
| def column_projection(collection) |
There was a problem hiding this comment.
This projects every Column field, including read-only ones — which the trail never captured.
AUDIT_TRAIL.md states the limitation correctly ("State reconstruction covers audited columns only — read-only, computed and DB-managed fields are never recorded, so they cannot be restored"), but this projection contradicts it: it selects all Column fields, so read-only ones are read at their current value and never reverted, then returned inside a "state at timestamp" answer. A caller sees updated_at as it is now, presented as it was then.
agent-nodejs#1686 restricts the equivalent to primary keys plus writable columns, with the reasoning attached: "Must mirror the audit log's column selection … so the reconstructed record carries the same columns the log captured."
Either restrict the projection to match what's audited, or mark the non-audited fields in the payload so the client can render them as "current, not historical". Silently mixing the two is the option to avoid.
Broader note: nothing calls /state on either agent, and the front's History tab doesn't either. Worth confirming something is planned for it — otherwise it's the cheapest surface to cut.
There was a problem hiding this comment.
The projection is fixed in 9185f92: audited_projection is primary keys plus writable columns, mirroring exactly what the capture layer records, so a "state at timestamp" answer no longer carries updated_at as it is now. AUDIT_TRAIL.md's stated limitation and the code agree again.
Your broader note is the one I cannot close from here, and it is a fair challenge: nothing calls /state on either agent, and the front's History tab doesn't. It is the cheapest surface to cut, and it brought its own weight — the absent-vs-nil diff encoding exists because a revert has to tell "key added" from "key holding null", which display alone never needed. That encoding is worth keeping regardless (it is strictly more truthful), but the routes and RecordState only pay for themselves if something is planned. Leaving that call to @bexchauveto.
Worth noting what it is not: it reconstructs, it does not restore. There is no write path, and a "Restore this version" button would go through the normal record update rather than a new endpoint, so it gets edit permission, validation, hooks and an audit row naming the operator who reverted — none of which a bespoke revert endpoint would get for free.
|
|
||
| # One row per targeted record, provisionally an `action` — {#confirm} settles which it really was. Returns | ||
| # the row ids to confirm. | ||
| def pending(caller:, collection:, action_name:, form_values:, record_ids:) |
| answer = summarize(result) | ||
|
|
||
| ids.each { |id| @store.confirm(id, operation: failed ? FAILED : EXECUTED, new_values: answer) } | ||
| end |
| end | ||
| end | ||
|
|
||
| audit_safely { @store.discard(empty.compact) } if empty.any? |
|
|
||
| # Reads the updated records back in one query, keyed by their own id, so each snapshot can find what | ||
| # actually landed — including when the patch moved a primary key. | ||
| def reread(context, records, patch, target) |
|
|
||
| # The one place the audit trail may refuse an operation, and only under `critical: true`: if we cannot | ||
| # record that a write is about to happen, the write does not happen. | ||
| def pending_rows(caller, operation, collection, rows) |
| # down with it still leaves evidence that it started. A failed run is worth recording too — "who tried | ||
| # to run this" is usually the interesting part — and an action answering with an Error result failed | ||
| # just as much as one that raised, it simply said so through `result_builder.error`. | ||
| def execute_and_audit(context, _args, data, filter) |
| model.where(id: ids).delete_all unless ids.empty? | ||
| end | ||
|
|
||
| def list_by_record(collection:, record_id:, skip: 0, limit: nil, user_ids: nil, start_timestamp: nil, |
| relation.map { |row| from_row(row) } | ||
| end | ||
|
|
||
| def count_by_record(collection:, record_id:, user_ids: nil, start_timestamp: nil, |
| # The distinct authors of the entries the current filters match, whatever page is being asked for. The | ||
| # identity comes from the rows themselves, so a user who has since been renamed or removed still reads | ||
| # as they were when they acted. | ||
| def authors_by_record(collection:, record_id:, user_ids: nil, start_timestamp: nil, |
| private | ||
|
|
||
| # Every filter is an AND, so the count matches exactly what a page of this history holds. | ||
| def scope(collection, record_id, user_ids: nil, start_timestamp: nil, end_timestamp: nil, |
| # (raw ISO strings with a `Z` would compare lexically against the cast rows and never match). | ||
| relation = relation.where('timestamp >= ?', as_time(start_timestamp)) if start_timestamp | ||
| relation = relation.where('timestamp <= ?', as_time(end_timestamp)) if end_timestamp | ||
| relation |
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>
…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>
`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>
…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>
…ubmitted An action row now uses both value columns for what they mean on that kind of row: `previous_values` is the submitted form, `new_values` what the action answered. No new column, no migration. The answer is an allowlist — type, message, name, mime_type, method, url, path — because a result also carries the file's contents, a webhook's body and headers and arbitrary response headers. File bytes have no business in an audit table, the other two routinely hold credentials, and an allowlist means a field added to a result later is not stored until someone decides it should be. `html` is left out as operator-facing markup the message already summarises. An action that raised stores no answer at all. This makes the state walk-back's handling of action rows load-bearing: their value columns are not a record's before and after, so applying either would corrupt a rebuild. The comment says so and the spec now feeds it a realistic action row. Also folds the row mapping onto the column list — an AuditRecord is a Struct and a row answers to `[]`, so the two 10-line field-by-field hashes were one list — and moves the migration list beside the runner that applies it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…icher rows The shared contract with the Node agent, plus the hardening that came with it. Schema, as new append-only migrations: the caller's first name, last name and email denormalised onto every row (who acted then, not who holds that id today); `action_name`; a not-null `status` backfilled to `done`; and `record_id` widened to TEXT and made nullable, since a create's pending row has no id yet and a packed composite id outgrows a varchar. Widening it rebuilds the indexes 002 created — sqlite drops them all with the table, and MySQL cannot index unbounded TEXT without a length prefix. Write protocol: a pending row before the write, confirmed after, one code path in both modes so `status` always means something. `config.audit_trail[:critical]` (default false) decides what a failed pending insert costs: the operation, or the row. What it buys is no unaudited write, not exact after-values everywhere — a row left pending says the write may or may not have landed, and that residue is evidence. A write that changed nothing has its row discarded instead. Also, per the review: - connect and migrate at boot rather than on first write, so a bad database stops the agent starting instead of recording nothing while looking healthy; and the class-level connection now refuses a second, different database rather than silently clobbering the first pool - one operation audits at most 500 records, batched, with `N audited, M skipped` logged — a "delete all" no longer materialises every row, twice - action results: url and path sanitised (userinfo, query, fragment), answer keys camelCased on write, and the targeted ids read back through the caller's filter instead of trusted from the request - the action's selection read and pending insert both sit inside the gate, so neither runs without an audit database nor 500s an action that already ran - /state projects primary keys plus writable columns only, matching what is actually recorded - the update diff is taken against the persisted record, and the row filed under the id the record ended up with - payload exposes the row `id`, and the history route carries `meta.availableUsers` on the first fetch Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing has shipped, so the six migrations collapse into the single create they would have produced: every column present from the start, `record_id` text and nullable, and the record_id index carrying MySQL's length prefix since the column is text from the outset. The tracker moves from a schema-wide `audit_migrations` to `audit_logs_migration`, named after the table it describes. One tracker per audited table is what the per-table key prefix was working around, so that goes too and migration names are plain again. `status` loses its default with it: the default only existed to backfill rows written before the column, and every write sets it — a row arriving without one is a bug worth a NOT NULL violation rather than a row quietly claiming to be done. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t is unknown Four review findings, all of them about claiming more than is known. - Under `critical: true`, a selection wider than the cap was audited in part while the write touched every match — the one thing that mode exists to prevent. It is refused now, before the write. A snapshot that cannot be read at all goes through the same gate, for the same reason: knowing what an operation is about to touch is part of being able to record it. - A record the agent cannot read back after an update left its row confirmed (or discarded) from the patch, claiming values that may never have been written. It stays pending, which is what that state means. - Reads no longer let a pending row act as fact: the state reconstruction skips them, since undoing a change that may never have happened would invent a state the record was never in. The history keeps them — they are evidence of an attempt, and `status` says so. - A caller with no request id gets no correlation key rather than a fabricated one. A generated uuid grouped the row into a request of its own, indistinguishable from a genuine single-row request. Also documents that a non-Rails host has to mount CorrelationIdMiddleware itself, since without its reset a pooled thread hands its previous key to the next request. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The flag was in the working tree without the two specs it changes. Its lookup now goes through `AuditTrail.store`, the same one the record-history route mounts itself on, so the capability cannot drift from what the routes actually serve — and the specs pin it both ways, since the front gates its History tab on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The activity logs can only match a term against their own description, so nothing finds the edit that set a city to Lyon. The agent is the only side holding the recorded values, so the record-history route takes an optional `search`. Matched case-insensitively, as a substring, against the action name, the actor's name and email, and the keys and values of both value objects at any depth — the JSON document as text, so one condition reaches every leaf and composes with pagination and the count instead of filtering in memory. Not matched against operation, correlation_key, record_id, collection, status or timestamp: machine identifiers nobody searches for, whose hits read as noise. Two things it has to get right. A masked field never matches — the `[redacted]` mask is stripped before comparison, and the value it hid was never recorded, so searching either finds nothing: a search must not confirm what the trail refused to keep. And LIKE wildcards in the term are escaped with `!` rather than a backslash, which MySQL also treats as an escape inside string literals. The per-adapter text cast lives in Sql::TextSearch beside the field filter, with its SQL pinned for Postgres and MySQL since the suite only runs SQLite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Filing an update under the record's new id — which later lookups use — left every row written before the rename under the old one, unreachable. So the history endpoint started the story at the rename, and /state reconstructed from half a timeline and called it the state. An update that moves a writable primary key now records the id it left, in `previous_record_id`, and both routes walk that chain back before querying: the current id plus every id the record has been filed under. Capped at ten hops, and a chain that comes back on itself stops rather than looping. The column stays out of the payload — it is how the agent follows a record, not part of the contract the front reads. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
683b52e to
55ef0eb
Compare
- a value holding a quote, a backslash or a newline was unfindable: the value objects are matched as serialized JSON, where those are escaped, so the term is escaped the same way. It also stops a bare quote matching the document's own structure, and so every row. - `insert_all ... RETURNING` order is not promised by Postgres, and the ids were paired with their rows positionally — one pending row could be confirmed with another record's diff. Matched by `record_id` now, which is unique within a batch, falling back to one insert per row when it is not. - an earlier id in a rename chain is now bounded by the moment it was left: a primary key a record abandons can be taken by another record, whose rows are none of this record's business. - the rename walk has no depth cap: subtracting the ids already held is both the cycle guard and the terminator, where a cap only truncated real chains. - `connect_to` puts the check, the connect and the assignment under one class-level mutex, so two stores racing cannot both pass the check with the loser writing to the winner's database. - `sanitize_url` strips credentials off a scheme-relative `//user:pass@host` too, which parses fine and kept them. - the action docs claimed recording is always best-effort, which stopped being true with `critical`. - documents that a row attached to no record — a global or over-cap action run — is recorded but not readable through any route today, and what the rename bound still cannot separate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
|
||
| establish_connection(database) | ||
| @database = database | ||
| end |
| '(record_id = ? AND timestamp <= ?)' | ||
| end | ||
|
|
||
| [sql.join(' OR '), *binds] |
| end | ||
| end | ||
|
|
||
| segments |
…he trail orders The bound knew only the timestamp, so a record taking an abandoned key inside the same millisecond as the rename landed in the previous occupant's history. The trail orders itself by (timestamp, id) — a bound in timestamps alone was a slightly different notion of "before" than the one it sorts by. `renamed_from` carries the rename row's id along, and a bounded segment reads `timestamp < t OR (timestamp = t AND id <= row)`. A caller that supplies only a timestamp still gets the inclusive form, so the simple case stays simple. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ot Comparable `pair[one] <= pair[other]` raised NoMethodError, so a record renamed twice took down the history and state routes rather than merely mis-bounding them. Array has `<=>` but none of the operators Comparable would have added. The specs never reached it: every case had at most one bounded segment, the other being the root's nil bound, which short-circuits before the comparison. Two now walk a chain of two renames — one where the older segment keeps its own bound, one where it inherits the earlier of the two — and they fail with the original NoMethodError against the previous code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PMerlet
left a comment
PMerlet
left a comment
There was a problem hiding this comment.
Second pass, against bf862d8. Everything from the first round is addressed, and several of the fixes here are the ones agent-nodejs#1686 should be copying rather than the reverse:
record_idist.text(and nullable for a pending create), with the index length-prefixed.sanitize_urlcoversurlandpath, andcamelize(:lower)on the result keys meansmimeTypenow matches Node instead ofmime_type— the front no longer sees two spellings.audit_pendingguards withreturn [] unless storeand runs the whole thing, record selection included, insideAuditTrail.gate. So nothing audit-related executes when the trail is off, and a raise no longer turns a successful action into a 500.audited_record_idsreads the ids back through the caller's own filter, so the trail no longer asserts an action touched a record the scope excluded.audited_projectionis restricted to primary keys plus writable columns, so/statestops mixing never-reverted read-only fields into a past state.correlation_key_forno longer invents a UUID.
Two of the fixes are strictly better than Node's equivalent and I've asked there to align on yours: fetching cap + 1 to distinguish "exactly cap" from "more than cap" (Node saturates at the cap and mis-reports an operation on exactly 1000 records), and filter.override(page: Page.new(...)) instead of spreading the filter into a plain object.
Three comments below, all cross-agent alignment rather than defects here.
| # every matched row and, with the pending/confirm protocol, write each of them twice. Truncation is logged, | ||
| # never silent. | ||
| # ponytail: 500 covers any hand-made bulk edit; raise it if a real workflow needs more. | ||
| MAX_RECORDS_PER_OPERATION = 500 |
There was a problem hiding this comment.
This cap is 500 here and 1000 in agent-nodejs#1686 (MAX_SNAPSHOT_RECORDS), and the two agents behave differently past it.
Same feature, same configuration surface, two thresholds: a bulk operation on 700 records is over the cap on a Ruby agent and fully audited on a Node one. And the overflow behaviour differs too — here the run is recorded as a single row attached to no record, there it's a logged warning (or a refusal of the whole operation under critical).
Since the front renders both agents identically and a customer can move between them, this wants to be one number and one documented behaviour. No opinion on which value — both are defensible; flagged on both PRs.
There was a problem hiding this comment.
Aligned in 87b05e9: the cap is 1000, matching MAX_SNAPSHOT_RECORDS, with a comment on the constant saying it moves in both agents or neither. I took Node's number rather than proposing a third, since it is the one already written down — say the word if 500 is the value you want and I will flip both.
Your second half caught something worse than the mismatch, though: the two paths inside this agent disagreed past the cap. A bulk write refused the operation under critical: true, while a smart action logged, recorded one row attached to no record, and ran anyway. So the mode's own invariant held for writes and not for actions. Both refuse now, through one method, since the gate sits ahead of execute and nothing has happened yet.
AUDIT_TRAIL.md now documents the overflow behaviour for both shapes and both modes in one place, so the next reader does not have to infer it from two call sites.
| # that an operation is about to happen, the operation is refused. Nothing was written, so there is nothing | ||
| # to repair and no compensating write ever happens. Default false keeps today's behaviour, where a broken | ||
| # audit database costs rows rather than writes. | ||
| def self.critical? |
There was a problem hiding this comment.
critical defaults to off, so the guarantee the pending/confirm protocol exists to provide is opt-in.
The two-phase protocol is what makes "no unaudited write" achievable, and with options[:critical] unset a failed pending insert is logged while the write proceeds unaudited. A customer who has configured an audit database — and therefore believes they have an audit trail — gets a best-effort one until they find a second flag. Configuring the database is already an explicit opt-in; requiring a second one to get the invariant puts the surprise in the wrong place.
Node defaults the same way, so this is a shared product call rather than a Ruby oversight — flagged in both places for that reason.
Whichever way it goes, AUDIT_TRAIL.md should say plainly that without critical a write can succeed with no audit row. It documents the protocol today without documenting that the guarantee is optional.
There was a problem hiding this comment.
Not changed, and deliberately not: you have named it a shared product call and Node defaults the same way, so flipping it here would trade a documented gap for a silent divergence in the direction that changes customer behaviour. That one belongs to @bexchauveto, with both agents moving together.
The part you asked for whichever way it goes is done in 87b05e9. AUDIT_TRAIL.md now says it outright rather than implying it:
The guarantee is opt-in.
criticaldefaults tofalse, so on a default configuration a write can succeed with no audit row at all — an unreachable audit database costs rows, not writes. Configuring the database gets you a best-effort trail;critical: trueis what makes "no unaudited write" true.
Worth adding for whoever makes the call: with critical: true the failure mode moves from "silently missing rows" to "refused operations" — an unreachable audit database stops writes, and a selection wider than the cap is refused outright (also newly true for smart actions, see the other thread). That is the right trade for a compliance deployment and the wrong one for someone who enabled the trail to get a nice Historic tab, which is the real argument for the default being a choice rather than a default.
There was a problem hiding this comment.
Decision, from @bexchauveto: the default stays false, in both agents. The reasoning, since it turns on something that changed after you wrote this.
Your argument rests on the customer who configures an audit database, believes they have an audit trail, and silently gets a best-effort one. That case is now covered by something else: the store connects and migrates at boot. A wrong connection string, an unreachable host, a migration that cannot run — the agent refuses to start. The "records nothing forever while looking healthy" scenario, which is the one that makes an opt-in guarantee feel like a trap, no longer exists.
What critical still governs is the transient failure: the audit database goes away mid-life. And that is where fail-closed is at its worst. It is a second database, usually a different host, on a connection nobody load-tested; with critical: true as the default, a blip there turns the admin panel read-only. That is an outage in the product caused by the subsystem whose only job is to observe it, and it lands hardest exactly when an extra database is most likely to be unhappy — during an incident, while people are using Forest to fix production.
The asymmetry is what settles it. false costs rows in a table nobody is reading yet, and the pending/confirm protocol you asked for is what makes that gap detectable rather than invisible: pending rows in the table, errors in the log. true costs writes, immediately, to the people least able to work out why the panel stopped saving.
So the shape is: configuring the database buys a best-effort trail with a visible failure mode; critical: true buys the invariant, and the docs now say so in the words you asked for, including which reader should set it.
Two things I would take from you here:
- If you think compliance deployments are the majority of adopters rather than a minority who read the flag, that flips the argument and both agents should move together. My read is the opposite — most people enabling this want the Historic tab — which is your own framing from the thread on recordless runs.
- The one concession that removes the rest of your objection without putting writes at risk: make the degraded state visible somewhere other than the log — a count of pending rows, or a line in the health check. Happy to build that here if you want it; it is the part of "the customer does not know" that survives the boot check.
| t.text :user_email | ||
| # Set only on an update that moved a writable primary key: the id the row was filed under | ||
| # before. What lets a history query follow a record across a rename. | ||
| t.text :previous_record_id |
There was a problem hiding this comment.
This column has no equivalent in agent-nodejs#1686 — previousRecordId appears nowhere there.
Following a record across a primary-key change is a genuine improvement and it goes further than Node, which files the update row under the record's new packed id but has no way to walk back past the rename. Since the front queries by the record's current id, the same renamed record shows a complete chain on a Ruby agent and a history that begins at the rename on a Node one.
Asked on the Node PR to port it. Raising it here so the decision is explicit either way — if rename-chaining stays Ruby-only, both READMEs should say so, because right now it reads as an accidental difference.
Unrelated, on the churn around bounding a rename segment (9bb4c5c, bf862d8): comparing bounds through <=> rather than relying on Array being Comparable is the right fix. Worth a short comment on the method saying why the pair is compared that way, so the next person doesn't reintroduce the array comparison.
There was a problem hiding this comment.
Recorded rather than removed, and documented as a divergence in 87b05e9:
A record that was renamed keeps one timeline — on this agent. The Node agent files an update under the record's new id too, but has no
previous_record_idand so cannot walk back past the rename: the same record shows a complete chain here and a history beginning at the rename there, until that column is ported.
Two things worth having from this side if the port happens, both learned the hard way in the last few commits. An earlier id has to be bounded by the moment it was left, not merely included — a primary key a record abandons can be taken by another record, whose rows are none of the first one's business. And that bound wants to be (timestamp, row id), the pair the trail already orders by, or two operations inside one millisecond fall on the wrong side of it.
On the comment you asked for: it is there, on earlier_bound — "Through <=>, since Array is not Comparable and <= on one raises." Agreed it needs to stay: the version before the pair comparison used [a, b].compact.min over ISO strings and was correct, because String is Comparable — which is exactly how the array comparison slipped in unnoticed.
…ion too The cap was 500 here and 1000 in the Node agent, so a bulk operation on 700 records was fully audited on one and truncated on the other, behind the same config key. Now 1000, with a comment saying it moves in both or neither. Past the cap the two paths inside this agent also disagreed: a bulk write refused the operation under `critical: true`, while a smart action recorded one row attached to no record and carried on. A partial audit is exactly what that mode exists to refuse, and the gate sits ahead of `execute`, so refusing costs nothing to repair. The message now lives in one place, since two paths raise it. AUDIT_TRAIL.md says plainly what it only implied: `critical` defaults to false, so on a default configuration a write can succeed with no audit row — configuring the database buys a best-effort trail, `critical: true` buys the invariant. It also records that rename-chaining is Ruby-only until `previous_record_id` is ported, rather than leaving it to read as an accident. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…est one Taking the newest entry off the stack is wrong as soon as writes nest: an inner write that fails and is rescued skips its after hook and stays there, so the outer hook confirmed the failed operation's rows as done and left its own stranded — lies in both directions. Entries are keyed by the object the hook decorator hands to both contexts, the filter or the data on a create, which is the identity the two hooks genuinely share. Where a customization replaced it there is nothing to match on: with one operation in flight that is unambiguous and still pairs; with several it does not guess, and the rows stay pending, which is what pending means. The specs built a fresh filter per hook call, so they modelled something production never does — that is why the case was reachable. They share one object per operation now, and three examples cover the nested failure, the replaced filter, and the ambiguous pair. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
# [1.39.0](v1.38.3...v1.39.0) (2026-08-19) ### Features * **audit_trail:** record and expose record history in the agent, gated on an audit database ([#320](#320)) ([dcfb13c](dcfb13c))
|
🎉 This PR is included in version 1.39.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
3 participants
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 with what was submitted and what came back; and five routes serve that history to the
Historic tab — filterable, searchable, and able to rebuild a record as it stood at any past instant.
It is off unless an audit database is configured. Nothing is loaded, no route is registered, no hook is
installed, and
canUseAuditTrailtells the front so:Why
This started as a separate plugin gem. Capturing changes needs the customizer hooks, the caller's per-request
id and the agent's route stack — agent internals a plugin had to reach into from outside, and users had to
wire the same store instance into two places by hand or silently get a half-working feature. Folding it in
turns that into one config key, and the capture layer is installed by the agent factory, so
reload!replaysit like any other customization.
ActiveRecord stays optional: the storage files are autoloaded on first use and excluded from Zeitwerk eager
loading, so agents without an audit database never load it. Outside Rails, add
gem 'activerecord'plus youradapter, and mount
CorrelationIdMiddlewareyourself.How it records
Every operation is written twice: a
pendingrow before the write, confirmeddoneafter it. One codepath either way, so
statusalways means the same thing.criticalfalsetrueWhat that buys is no unaudited write, not that every row holds exact after-values. A row left
pendingmeans the write may or may not have landed; that residue is evidence, and it is the point.
The default is
false, decided rather than inherited. Connecting and migrating at boot already means amisconfigured audit database stops the agent starting, so what
criticalstill governs is a transientfailure — and there, fail-closed turns a blip in a second database into a read-only admin panel, exactly when
that database is least happy and Forest is the tool being used to fix production.
falsecosts rows in atable nobody reads yet, and the protocol makes that gap detectable;
truecosts writes, immediately, to thepeople least able to diagnose it. Compliance deployments set
critical: true, which the README says plainly. Everything afterthe pending insert is best-effort in both modes, because by then the write has happened and raising would
report a failure for an operation that succeeded.
Consequences worth knowing: a write that changed nothing has its row discarded; a record the agent cannot
read back keeps its row pending rather than being confirmed from the patch; one operation audits at most 500
records, truncated with
N audited, M skippedlogged undercritical: falseand refused undertrue;and pending rows stay visible in the history —
statussays what they are — but the state reconstructionignores them, since undoing a change that may never have happened would invent a state the record was never
in.
The diff is taken against the record as persisted, not the requested patch, so normalisation and decorator
side effects are what gets recorded. An update that moves a writable primary key files its row under the
record's new id and remembers the one it left, so history follows a record across a rename — each earlier id
counted only up to the moment it was left, by
(timestamp, id), the pair the trail orders itself by.What the routes serve
All under
/forest/_audit-trail, all requiringcan?(:read, collection)and the caller's permissionscope on the target record, in one query rather than a check followed by an unscoped read.
GET /_audit-trail/{collection}/{id}— one page of history. Filters:userIds,startDate/endDate(wall-clock in the request
timezone),fields, andsearch.meta.countreflects every filter;meta.availableUsersrides along on the first fetch only.GET /_audit-trail/{collection}/{id}/state?timestamp=— the record as it stood then, rebuilt by undoingevery entry recorded strictly after that instant. Matches Node's
handleStateAt:{ data }, nothing else.GET /_audit-trail/correlation/{key},GET|POST /_audit-trail/correlations— history grouped by theper-request correlation key.
searchis what only the agent can answer: matched case-insensitively, as a substring, against the actionname, the actor's name and email, and the keys and values of both value objects at any depth — so
Lyonfinds
{"address": {"city": "Lyon"}}. In SQL, so it composes with pagination and the count. Not matchedagainst
operation,correlationKey,recordId,collection,statusortimestamp: machine identifierswhose hits read as noise. A redacted field never matches, by its mask or by the value it hid.
The per-adapter JSON work for
searchandfieldslives inSql::TextSearchandSql::FieldFilter:Postgres (
jsonb_object_keys,::text), SQLite (json_type, as stored), MySQL / MariaDB(
JSON_CONTAINS_PATH,CAST … AS CHAR), and a clear raise on anything else. The suite runs SQLite, so theother two are pinned by their own specs — including that neither emits a bind placeholder, that a term's LIKE
wildcards are escaped, and that a field name holding a dot stays one key.
Payload
Top-level keys are camelCase:
id,timestamp,operation,collection,recordId,status,userId,userFirstName,userLastName,userEmail,actionName,correlationKey,previousValues,newValues.The row
idis exposed because both agents order by(timestamp, id)and the front uses it as the mergetiebreaker. The actor's name and email are denormalised at write time, so a row says who acted then.
Inside the value objects: a record's column names pass through untouched, while an action answer's keys are
Forest's own and therefore camelCase (
mimeType). An action's answer is an allowlist —type,message,name,mimeType,method,url,path— because a result also carries the file's contents, a webhook'sbody and headers, and arbitrary response headers;
urlandpathare stripped of userinfo, query andfragment before storage.
A nested key absent on one side is left out of that side rather than written as
null, so{"flag": null}and{}stay tellable apart. That is what lets the state route remove a key a change had added insteadof resurrecting it as
null, and it keeps sentinel values out of the database.Limitations, stated on purpose
Customer.find(id).update!(...)is invisible to the agent; only its invocation row exists.previousValues. Hooks bracket the write as separate calls and the datalayer exposes no lock, deliberately, since it spans ActiveRecord, Mongoid and HTTP APIs.
newValuesstaysexact and no row is lost. Exact before-images under concurrency need triggers or CDC.
arrived — a reused key, or a delete followed by a recreate — cannot be told apart from this record's own.
Deliberate for delete/recreate, which the reconstruction walks into on purpose; the same limitation reached
another way for a reused key. Separating them needs a lineage id on every row, and a read per audited write.
recordId, and every route is record-scoped, so they are evidence in the table that no consumer can fetch.the caller's scope. Once no record exists there is nothing to evaluate a scope against.
recorded, so they cannot be restored.
redactmasks values but still records the change, and applies to smart-action form values too.Open questions
Four calls that belong to a human rather than to this branch:
deleted record to anyone with collection read permission — deleting a record widens who can read it.
@PMerlet's middle option: surface that a deletion happened, by whom and when, while withholding the
deleterow'spreviousValuesfrom out-of-scope callers. Either way it wants to be in the ticket and thecustomer-facing docs, not only a source comment.
/statewanted? Nothing calls it on either agent, and the front's History tab doesn't. It is thecheapest surface to cut. The absent-vs-nil encoding it forced is worth keeping regardless.
limitation acceptable?
Notes
No new package, so nothing was added to
.releaserc.jsor the CI matrices; the audit trail ships withforest_admin_agent. The migration list is a single create, since nothing has been released — tracked inaudit_logs_migration, named after the table it builds, so two stores with differenttable_names keepseparate histories. Out of scope but spotted on the way:
Utils::QueryStringParser#parse_paginationdigs intoparams[:page]assuming a Hash, so?page=fooraisesfor every list/count route.
🤖 Generated with Claude Code
Note
Add audit trail feature to record and expose record history gated on an audit database
AuditTrail::Store(PostgreSQL, SQLite, MySQL/MariaDB)./_audit-trail/:collection/:idand/_audit-trail/:collection/:id/state) and correlation-key endpoints (/_audit-trail/correlation/:key,/_audit-trail/correlations), all enforcing per-record scope checks.Migratorthat creates the audit schema and applies versioned, idempotent DDL migrations; on PostgreSQL it serializes concurrent runs with a transaction-scoped advisory lock.CorrelationIdmodule andCorrelationIdMiddlewarethat lazily generate a per-request UUID, attach it to responses asx-forest-correlation-id, and propagate it intoCallerfor cross-operation linking.databaseis configured; the capabilities endpoint advertisescanUseAuditTrail: trueonly when a store is connected.MAX_RECORDS_PER_OPERATIONare truncated (or raise incriticalmode), which may block writes when critical mode is enabled.Changes since #320 opened
RETURNINGwithrecord_idmapping instead of relying on insertion order [22cdb7b]until _rowin audit trail store and route specifications [9bb4c5c]Routes::Resources::AuditTrailRoute.earlier_boundmethod [bf862d8]forest_admin_agentaudit trail to use object identity matching instead of implicit stack-based pairing [fcaa198]forest_admin_agentto reflect object-identity-based snapshot pairing behavior [fcaa198]Macroscope summarized 55ef0eb.