refactor(audit): derive updatedFields through one shared helper - #6604
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryLow Risk Overview Also renames the admin organization update audit metadata key from Reviewed by Cursor Bugbot for commit 9c6d5b6. Configure here. |
Greptile SummaryThe PR centralizes audit updated-field derivation in
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/audit/src/updated-fields.ts | Introduces the shared helper that returns written keys while excluding updatedAt. |
| apps/sim/app/api/v1/admin/organizations/[id]/route.ts | Reuses one derived field list for logging and standardized organization audit metadata. |
| apps/sim/lib/credentials/orchestration/index.ts | Replaces inline audit-field derivation with the shared helper. |
| apps/sim/lib/mcp/orchestration/server-lifecycle.ts | Uses the shared helper for updated fields returned from MCP server updates. |
| apps/sim/lib/mcp/orchestration/workflow-mcp-lifecycle.ts | Uses the shared helper in workflow MCP server and tool update audit paths. |
| packages/testing/src/mocks/audit.mock.ts | Exposes auditUpdatedFields through the shared audit test mock. |
Reviews (2): Last reviewed commit: "test(audit): pin the testing mock's copy..." | Re-trigger Greptile
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.
@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.
1257a97 to
9c6d5b6
Compare
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 9c6d5b6. Configure here.
Summary
Object.keys(updateData).filter(k => k !== 'updatedAt')across four files each decided independently which columns an audit row reports. TheupdatedAtexclusion is an audit convention, not a local detail, so it moves to@sim/auditasauditUpdatedFieldsand becomes a single editv1/admin/organizations/[id]/route.tsevaluated the expression twice in one handler and filed it under the metadata keyfields, while every other site usesupdatedFields. Any consumer filtering onupdatedFieldssilently missed org updates. Verified nothing readsmetadata.fieldsbefore renamingSites converted
lib/mcp/orchestration/server-lifecycle.ts:403lib/mcp/orchestration/workflow-mcp-lifecycle.ts:552, :939lib/credentials/orchestration/index.ts:339app/api/v1/admin/organizations/[id]/route.ts:182, :193fieldsTesting-mock note
auditMockcarries the real implementation, not avi.fn()stub — callers under test derive their audit metadata through it, so stubbing would erase exactly what those tests assert. The two suites that hand-roll their own@sim/auditfactory (credentials/orchestration,workflow-mcp-lifecycle) source it fromauditMockrather than re-inlining, so there is still one implementation.Type of Change
Testing
packages/audit/src/updated-fields.test.ts: written columns,updatedAtdropped, explicitly-nullcolumns kept (clearing a value is a change), and the only-updatedAtempty caselib/mcp,lib/credentials,lib/copilot/tools/handlers/deployment,app/api/mcp,app/api/v1/admin,app/api/v2/mcp; 29 inpackages/auditlint:checkandcheck:api-validation:strictpass; typecheck cleanLocal-verification caveat worth knowing: this was developed in a git worktree whose
node_modulessymlinks to the main checkout, so@sim/auditand@sim/testingresolve to the main copy and a new package export is invisible to the worktree's test run. Every result above was produced by temporarily materializing the two package files into the main checkout, running, then restoring it — otherwise the run reports phantom failures that look like this change breaking 100+ audit-mocking suites.Checklist