feat(domain): add configuration audit trail with diff detection and rollback#796
Conversation
…ollback Add configAudit module that records every change to provider connections, combos, and routing policies with: - Before/after state snapshots - Structured diff (added, removed, changed keys) - Source tracking (dashboard, API, sync, auto-healing) - Filtered retrieval with pagination - Rollback state extraction - Configuration snapshot export for backup Enables traceability and quick rollback when config changes cause issues. Closes diegosouzapw#791
Code Review SummaryStatus: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
| | Files Reviewed (1 file)
|
There was a problem hiding this comment.
Code Review
This pull request introduces a configuration audit trail system in src/domain/configAudit.ts to track changes to entities like providers and policies with before/after snapshots. Key feedback includes replacing the counter-based ID generation with UUIDs for better reliability, using a more robust deep comparison method instead of JSON.stringify for computing diffs, and optimizing the in-memory log bounding logic by using shift() instead of slice().
| let idCounter = 0; | ||
|
|
||
| function generateId(): string { | ||
| idCounter++; | ||
| const ts = Date.now().toString(36); | ||
| const seq = idCounter.toString(36).padStart(4, "0"); | ||
| return `audit-${ts}-${seq}`; | ||
| } |
There was a problem hiding this comment.
The current generateId implementation using a simple idCounter is not robust. It's not guaranteed to be unique across application restarts or in a concurrent environment. A better approach is to use a universally unique identifier.
I suggest using crypto.randomUUID(), which is built into modern Node.js versions (v14.17.0+). This removes the need for idCounter and is much more reliable.
| let idCounter = 0; | |
| function generateId(): string { | |
| idCounter++; | |
| const ts = Date.now().toString(36); | |
| const seq = idCounter.toString(36).padStart(4, "0"); | |
| return `audit-${ts}-${seq}`; | |
| } | |
| // idCounter is no longer needed when using UUIDs. | |
| function generateId(): string { | |
| // Using crypto.randomUUID() for globally unique identifiers. | |
| // This is more robust than a timestamp-based counter. | |
| // Add `import { randomUUID } from 'node:crypto';` to file imports. | |
| return `audit-${randomUUID()}`; | |
| } |
| if (afterKeys.has(key)) { | ||
| const beforeVal = before![key]; | ||
| const afterVal = after![key]; | ||
| if (JSON.stringify(beforeVal) !== JSON.stringify(afterVal)) { |
There was a problem hiding this comment.
Using JSON.stringify for object comparison can be unreliable. The order of object keys is not guaranteed in JavaScript, which can lead to different JSON strings for semantically identical objects (e.g., {a:1, b:2} vs {b:2, a:1}).
For more robust deep object comparison, consider using a library like lodash.isEqual.
| if (auditLog.length > 1000) { | ||
| auditLog = auditLog.slice(-1000); | ||
| } |
There was a problem hiding this comment.
To keep the log bounded, the current code uses auditLog.slice(-1000). This creates a new array of up to 1000 elements every time an entry is added beyond the limit, which can be inefficient. Since entries are added one by one, a more performant approach is to simply remove the oldest element using auditLog.shift().
| if (auditLog.length > 1000) { | |
| auditLog = auditLog.slice(-1000); | |
| } | |
| if (auditLog.length > 1000) { | |
| auditLog.shift(); | |
| } |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94a5e43e5d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| before, | ||
| after, |
There was a problem hiding this comment.
Clone before/after snapshots when recording changes
recordChange stores before and after by reference, so later mutations to the original config objects rewrite historical audit entries and invalidate rollback data. This breaks the core audit-trail guarantee in common flows where callers reuse mutable config objects (e.g., update object then pass it through additional transformations). Store deep-cloned snapshots at write time so history remains immutable.
Useful? React with 👍 / 👎.
| export function getRollbackState(entryId: string): Record<string, unknown> | null { | ||
| const entry = getAuditEntry(entryId); | ||
| if (!entry) return null; | ||
| return entry.before; |
There was a problem hiding this comment.
Return a copy from rollback state access
getRollbackState returns the internal entry.before object directly, which lets callers mutate the audit log by editing the returned value. After one rollback attempt that modifies this object, future rollbacks and log inspection can be silently corrupted. Return a cloned snapshot instead of the in-memory reference.
Useful? React with 👍 / 👎.
| if (afterKeys.has(key)) { | ||
| const beforeVal = before![key]; | ||
| const afterVal = after![key]; | ||
| if (JSON.stringify(beforeVal) !== JSON.stringify(afterVal)) { |
There was a problem hiding this comment.
Use structural compare instead of JSON stringify diff
Diff detection currently uses JSON.stringify equality, which is order-sensitive for object keys. Two semantically identical configs with different key insertion order will be marked as changed, producing noisy or misleading audit entries and potentially triggering unnecessary rollback decisions. Use a stable deep-equality check that ignores key ordering for plain objects.
Useful? React with 👍 / 👎.
|
Olá, Professor Igor! Muito obrigado pela iniciativa fenomenal! 🕵️♂️📝 Manter um log de auditoria (Audit Trail) contra mudanças que possam quebrar as políticas e o fluxo de proxy é uma feature muito aguardada, especialmente pensando em rollbacks de configurações falhas! Para transformarmos este conceito genial em uma feature de produção pronta para o OmniRoute, analise as sugestões abaixo para ajustarmos este Pull Request: Pontos de Adaptação (Padrão de Arquitetura)
Por favor, faça essas adições arquiteturais e atualize sua branch |
Summary
configAudit.tstosrc/domain/getRollbackState(entryId)Motivation
When a routing config change causes failures, there's currently no way to:
This is critical after provider cleanups (disabling dead connections) or combo reordering — operations that can silently break routing.
API
Test plan
Relates to #791
🤖 Generated with Claude Code