refactor(audit): derive updatedFields through one shared helper (#6604)

* refactor(audit): derive updatedFields through one shared helper

Six copies of Object.keys(updateData).filter(k => k !== 'updatedAt') across
four files decided, independently, which columns an audit row reports. The
exclusion set is an audit convention, not a local detail, so it moves to
@sim/audit as auditUpdatedFields and the exclusion becomes a single edit.

The admin organizations route evaluated the expression twice in one handler
and filed it under the metadata key `fields` while every other site uses
`updatedFields`, so any consumer filtering on updatedFields silently missed
org updates. It now computes once and uses the shared key; nothing reads
metadata.fields.

auditMock carries the real implementation rather than a stub, since callers
under test derive their audit metadata through it. The two suites that
hand-roll an @sim/audit factory source it from there.

* test(audit): pin the testing mock's copy of auditUpdatedFields

@sim/audit devDepends on @sim/testing, so the mock cannot import the real
helper without closing a package cycle. Assert parity from the audit side
instead, where the dependency already runs the safe direction, so a change to
the exclusion convention cannot leave mocked callers validating behavior the
deployed helper no longer has.

Author: waleedlatif1

apps/sim/app/api/v1/admin/organizations/[id]/route.ts

@@ -30,7 +30,13 @@
  * Response: AdminSingleResponse<{ success, organizationId, slug, membersRemoved, workspacesDetached }>
  */
 
-import { AuditAction, AuditResourceType, recordAudit, recordAuditBatch } from '@sim/audit'
+import {
+  AuditAction,
+  AuditResourceType,
+  auditUpdatedFields,
+  recordAudit,
+  recordAuditBatch,
+} from '@sim/audit'
 import { db } from '@sim/db'
 import { member, organization, subscription } from '@sim/db/schema'
 import { createLogger } from '@sim/logger'
@@ -178,9 +184,8 @@ export const PATCH = withRouteHandler(
         .where(eq(organization.id, organizationId))
         .returning()
 
-      logger.info(`Admin API: Updated organization ${organizationId}`, {
-        fields: Object.keys(updateData).filter((k) => k !== 'updatedAt'),
-      })
+      const updatedFields = auditUpdatedFields(updateData)
+      logger.info(`Admin API: Updated organization ${organizationId}`, { updatedFields })
 
       recordAudit({
         workspaceId: null,
@@ -190,7 +195,7 @@ export const PATCH = withRouteHandler(
         resourceId: organizationId,
         resourceName: updated.name,
         description: `Admin API updated organization "${updated.name}"`,
-        metadata: { fields: Object.keys(updateData).filter((k) => k !== 'updatedAt') },
+        metadata: { updatedFields },
         request,
       })
 

apps/sim/lib/credentials/orchestration/index.test.ts

@@ -1,7 +1,13 @@
 /**
  * @vitest-environment node
  */
-import { dbChainMockFns, queueTableRows, resetDbChainMock, schemaMock } from '@sim/testing'
+import {
+  auditMock,
+  dbChainMockFns,
+  queueTableRows,
+  resetDbChainMock,
+  schemaMock,
+} from '@sim/testing'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
 
 const {
@@ -26,6 +32,7 @@ vi.mock('@sim/audit', () => ({
   AuditAction: { CREDENTIAL_UPDATED: 'credential.updated' },
   AuditResourceType: { CREDENTIAL: 'credential' },
   recordAudit: mockRecordAudit,
+  auditUpdatedFields: auditMock.auditUpdatedFields,
 }))
 vi.mock('@/lib/credentials/access', () => ({
   getCredentialActorContext: mockGetCredentialActorContext,

apps/sim/lib/credentials/orchestration/index.ts

@@ -1,4 +1,4 @@
-import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit'
+import { AuditAction, AuditResourceType, auditUpdatedFields, recordAudit } from '@sim/audit'
 import { db } from '@sim/db'
 import { credential, environment, webhook, workspaceEnvironment } from '@sim/db/schema'
 import { createLogger } from '@sim/logger'
@@ -336,7 +336,7 @@ export async function performUpdateCredential(
         .where(and(eq(webhook.provider, 'slack'), eq(webhook.routingKey, params.credentialId)))
     }
 
-    const updatedFields = Object.keys(updates).filter((key) => key !== 'updatedAt')
+    const updatedFields = auditUpdatedFields(updates)
     recordAudit({
       workspaceId: access.credential.workspaceId,
       actorId: params.userId,

apps/sim/lib/mcp/orchestration/server-lifecycle.ts

@@ -1,4 +1,4 @@
-import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit'
+import { AuditAction, AuditResourceType, auditUpdatedFields, recordAudit } from '@sim/audit'
 import { db, mcpServers } from '@sim/db'
 import { mcpServerOauth } from '@sim/db/schema'
 import { createLogger } from '@sim/logger'
@@ -400,7 +400,7 @@ export async function updateMcpServer(
       success: true,
       server,
       configurationChanged: shouldClearCache,
-      updatedFields: Object.keys(updateData).filter((key) => key !== 'updatedAt'),
+      updatedFields: auditUpdatedFields(updateData),
     }
   } catch (error) {
     logger.error('Failed to update MCP server', { error })

apps/sim/lib/mcp/orchestration/workflow-mcp-lifecycle.test.ts

@@ -1,7 +1,7 @@
 /**
  * @vitest-environment node
  */
-import { dbChainMock, dbChainMockFns, resetDbChainMock, schemaMock } from '@sim/testing'
+import { auditMock, dbChainMock, dbChainMockFns, resetDbChainMock, schemaMock } from '@sim/testing'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
 
 vi.mock('@sim/audit', () => ({
@@ -14,6 +14,7 @@ vi.mock('@sim/audit', () => ({
     MCP_TOOL: 'mcp_tool',
   },
   recordAudit: vi.fn(),
+  auditUpdatedFields: auditMock.auditUpdatedFields,
 }))
 vi.mock('@sim/db', () => ({
   ...dbChainMock,

apps/sim/lib/mcp/orchestration/workflow-mcp-lifecycle.ts

@@ -1,4 +1,4 @@
-import { AuditAction, AuditResourceType, recordAudit } from '@sim/audit'
+import { AuditAction, AuditResourceType, auditUpdatedFields, recordAudit } from '@sim/audit'
 import { db, workflow, workflowMcpServer, workflowMcpTool } from '@sim/db'
 import { createLogger } from '@sim/logger'
 import { generateId } from '@sim/utils/id'
@@ -549,7 +549,7 @@ export async function performUpdateWorkflowMcpServer(
   if (params.description !== undefined) updateData.description = params.description?.trim() || null
   if (params.isPublic !== undefined) updateData.isPublic = params.isPublic
 
-  const updatedFields = Object.keys(updateData).filter((key) => key !== 'updatedAt')
+  const updatedFields = auditUpdatedFields(updateData)
 
   try {
     const [server] = await db
@@ -936,7 +936,7 @@ export async function performUpdateWorkflowMcpTool(
       updateData.parameterSchema = applyDescriptionOverrides(baseSchema, overrides)
     }
 
-    const updatedFields = Object.keys(updateData).filter((key) => key !== 'updatedAt')
+    const updatedFields = auditUpdatedFields(updateData)
 
     const tool = await db.transaction(async (tx) => {
       await acquireWorkflowMcpServerLock(tx, params.serverId)

packages/audit/src/index.ts

@@ -1,3 +1,4 @@
 export { recordAudit, recordAuditBatch } from './log'
 export type { AuditActionType, AuditResourceTypeValue } from './types'
 export { AuditAction, AuditResourceType } from './types'
+export { auditUpdatedFields } from './updated-fields'

packages/audit/src/updated-fields.test.ts

@@ -0,0 +1,46 @@
+import { auditMock } from '@sim/testing'
+import { describe, expect, it } from 'vitest'
+import { auditUpdatedFields } from './updated-fields'
+
+describe('auditUpdatedFields', () => {
+  it('returns the written columns', () => {
+    expect(auditUpdatedFields({ name: 'Renamed', url: 'https://example.com' })).toEqual([
+      'name',
+      'url',
+    ])
+  })
+
+  it('drops updatedAt, which every write moves', () => {
+    expect(auditUpdatedFields({ name: 'Renamed', updatedAt: new Date() })).toEqual(['name'])
+  })
+
+  it('keeps columns explicitly written as null — clearing a value is a change', () => {
+    expect(auditUpdatedFields({ lastConnected: null, lastError: null })).toEqual([
+      'lastConnected',
+      'lastError',
+    ])
+  })
+
+  it('returns an empty list when only updatedAt was written', () => {
+    expect(auditUpdatedFields({ updatedAt: new Date() })).toEqual([])
+  })
+
+  /**
+   * `auditMock` carries its own copy because `@sim/audit` devDepends on
+   * `@sim/testing` — importing the real helper there would close a package
+   * cycle. The copy is pinned from this side instead, where the dependency
+   * already runs the safe direction.
+   */
+  it('stays in step with the copy @sim/testing hands to mocked callers', () => {
+    const cases: object[] = [
+      { name: 'Renamed', url: 'https://example.com' },
+      { name: 'Renamed', updatedAt: new Date() },
+      { lastConnected: null, lastError: null },
+      { updatedAt: new Date() },
+      {},
+    ]
+    for (const updateValues of cases) {
+      expect(auditMock.auditUpdatedFields(updateValues)).toEqual(auditUpdatedFields(updateValues))
+    }
+  })
+})

packages/audit/src/updated-fields.ts

@@ -0,0 +1,15 @@
+/**
+ * Columns a write touched, ready for an audit row's `updatedFields` metadata.
+ *
+ * Pass the update object the write actually applied — never the caller's
+ * params. A param is not a write: an unchanged value still arrives, and a
+ * writer often sets columns nobody asked for (a status reset forced by some
+ * other change). Deriving names from input is what makes audit rows name fields
+ * that were never written and omit the ones that were.
+ *
+ * `updatedAt` is excluded because every write moves it, so it is noise in every
+ * row. Keeping that rule here means it is one edit if the set ever grows.
+ */
+export function auditUpdatedFields(updateValues: object): string[] {
+  return Object.keys(updateValues).filter((key) => key !== 'updatedAt')
+}

packages/testing/src/mocks/audit.mock.ts

@@ -28,6 +28,12 @@ export const auditMockFns = {
 export const auditMock = {
   recordAudit: auditMockFns.mockRecordAudit,
   recordAuditBatch: auditMockFns.mockRecordAuditBatch,
+  /**
+   * Real implementation, not a stub: callers under test derive their audit
+   * metadata through it, so stubbing it would erase what the test asserts.
+   */
+  auditUpdatedFields: (updateValues: object): string[] =>
+    Object.keys(updateValues).filter((key) => key !== 'updatedAt'),
   AuditAction: {
     API_KEY_CREATED: 'api_key.created',
     API_KEY_UPDATED: 'api_key.updated',