fix(mergedeep): preserve top-level array diff shapes

lib/mergeDeep.js

@@ -89,7 +89,8 @@ class MergeDeep {
    * @param {*} additions aggregated so far
    * @param {*} modifications aggregated so far
    * @param {*} deletions aggregated so far
-   * @returns object with additions, modifications, and deletions
+   * @returns object with additions, modifications, deletions, and hasChanges.
+   * Array sources return array change buckets; object sources return object buckets.
    */
   compareDeep (t, s, additions, modifications, deletions, parentKey) {
     // Preemtively return if the source is not an object or array
@@ -107,21 +108,22 @@ class MergeDeep {
 
     // Also initialize the additions, modifications, and deletions for the first invocation
     if (firstInvocation) {
-      if (Array.isArray(source)) {
-        additions = []
-        modifications = []
-        deletions = []
-      } else {
-        additions = {}
-        modifications = {}
-        deletions = {}
-      }
+      additions = {}
+      modifications = {}
+      deletions = {}
     }
 
     // If the target is empty, then all the source is added to additions
     if (t === undefined || t === null || (this.isEmpty(t) && !this.isEmpty(s))) {
-      additions = Object.assign(additions, s)
-      return ({ additions, modifications, hasChanges: true })
+      if (firstInvocation && Array.isArray(s)) {
+        return { additions: s.slice(), modifications: [], deletions: [], hasChanges: true }
+      }
+      // Preserve recursive accumulator references without copying unsafe keys.
+      for (const key of Object.keys(s)) {
+        if (key === '__proto__' || key === 'constructor') continue
+        additions[key] = s[key]
+      }
+      return ({ additions, modifications, deletions, hasChanges: true })
     }
 
     // Compare the entries in the objects or elements of the array
@@ -208,17 +210,11 @@ class MergeDeep {
         }
       }
     }
-    // Unwind the topleve array from the object
-    if (firstInvocation) {
-      if (additions.__array) {
-        additions = additions.__array
-      }
-      if (modifications.__array) {
-        modifications = modifications.__array
-      }
-      if (deletions.__array) {
-        deletions = deletions.__array
-      }
+    // Unwind only wrapped top-level arrays, including buckets pruned as empty.
+    if (firstInvocation && Array.isArray(s)) {
+      additions = additions.__array || []
+      modifications = modifications.__array || []
+      deletions = deletions.__array || []
     }
     return ({ additions, modifications, deletions, hasChanges: !this.isEmpty(additions) || !this.isEmpty(modifications) || !this.isEmpty(deletions) })
   }

test/unit/lib/mergeDeep.test.js

@@ -4,6 +4,119 @@ const YAML = require('js-yaml')
 const log = require('pino')('test.log')
 
 describe('MergeDeep Test', () => {
+  describe('compareDeep result shape', () => {
+    const existing = { name: 'existing', enforcement: 'active' }
+    const added = { name: 'new', enforcement: 'active' }
+    const updated = { name: 'existing', enforcement: 'disabled' }
+
+    it.each([
+      ['first entry', [], [added], [added], [], [], true],
+      ['undefined target', undefined, [added], [added], [], [], true],
+      ['null target', null, [added], [added], [], [], true],
+      ['empty object target', {}, [added], [added], [], [], true],
+      ['empty arrays', [], [], [], [], [], false],
+      ['undefined target and empty source', undefined, [], [], [], [], true],
+      ['null target and empty source', null, [], [], [], [], true],
+      ['empty object and empty array', {}, [], [], [], [], false],
+      ['unchanged entries', [existing], [existing], [], [], [], false],
+      ['reordered entries', [existing, added], [added, existing], [], [], [], false],
+      ['subsequent entry', [existing], [existing, added], [added], [], [], true],
+      ['modified entry', [existing], [updated], [], [updated], [], true],
+      ['deleted entry', [existing], [], [], [], [existing], true],
+      ['replacement entry', [existing], [added], [added], [], [existing], true],
+      ['primitive entries', ['a'], ['a', 'b'], ['b'], [], [], true]
+    ])('returns array buckets for %s', (_name, target, source, additions, modifications, deletions, hasChanges) => {
+      const mergeDeep = new MergeDeep(log, jest.fn())
+
+      expect(mergeDeep.compareDeep(target, source)).toStrictEqual({
+        additions, modifications, deletions, hasChanges
+      })
+    })
+
+    it.each([undefined, null, {}, []])('preserves object additions with an empty target %p', target => {
+      const source = { name: 'policy', enabled: false, count: 0, rules: [added] }
+      const mergeDeep = new MergeDeep(log, jest.fn())
+
+      expect(mergeDeep.compareDeep(target, source)).toStrictEqual({
+        additions: source,
+        modifications: {},
+        deletions: {},
+        hasChanges: true
+      })
+    })
+
+    it('does not treat omitted object metadata as deletions', () => {
+      const mergeDeep = new MergeDeep(log, jest.fn())
+
+      expect(mergeDeep.compareDeep({ name: 'policy', id: 42 }, { name: 'policy' })).toStrictEqual({
+        additions: {}, modifications: {}, deletions: {}, hasChanges: false
+      })
+      expect(mergeDeep.compareDeep({}, {})).toStrictEqual({
+        additions: {}, modifications: {}, deletions: {}, hasChanges: false
+      })
+    })
+
+    it.each([undefined, null, [], [existing]])('preserves nested array additions for target %p', rules => {
+      const source = { rules: rules?.length ? [existing, added] : [added] }
+      const mergeDeep = new MergeDeep(log, jest.fn())
+
+      expect(mergeDeep.compareDeep({ rules }, source)).toStrictEqual({
+        additions: { rules: [added] },
+        modifications: {},
+        deletions: {},
+        hasChanges: true
+      })
+    })
+
+    it('does not unwind a user-defined __array property in an object comparison', () => {
+      const mergeDeep = new MergeDeep(log, jest.fn())
+
+      expect(mergeDeep.compareDeep({ __array: [] }, { __array: [added] })).toStrictEqual({
+        additions: { __array: [added] },
+        modifications: {},
+        deletions: {},
+        hasChanges: true
+      })
+    })
+
+    it.each([undefined, null, {}])('skips unsafe own keys when copying an empty target %p', target => {
+      const source = JSON.parse('{"name":"policy","__proto__":{"polluted":true},"constructor":{"polluted":true}}')
+      const mergeDeep = new MergeDeep(log, jest.fn())
+      const result = mergeDeep.compareDeep(target, source)
+
+      expect(result).toStrictEqual({
+        additions: { name: 'policy' }, modifications: {}, deletions: {}, hasChanges: true
+      })
+      expect(Object.getPrototypeOf(result.additions)).toBe(Object.prototype)
+      expect(Object.hasOwn(result.additions, 'constructor')).toBe(false)
+    })
+
+    it.each([{}, []])('keeps recursive accumulator references for %p', additions => {
+      const mergeDeep = new MergeDeep(log, jest.fn())
+      const source = Array.isArray(additions) ? [added] : { rules: [added] }
+      const modifications = Array.isArray(additions) ? [] : {}
+      const deletions = Array.isArray(additions) ? [] : {}
+      const result = mergeDeep.compareDeep(undefined, source, additions, modifications, deletions)
+
+      expect(result).toStrictEqual({ additions: source, modifications, deletions, hasChanges: true })
+      expect(result.additions).toBe(additions)
+      expect(result.modifications).toBe(modifications)
+      expect(result.deletions).toBe(deletions)
+    })
+
+    it.each([[[]], [[existing]]])('does not mutate inputs for target %p', entries => {
+      const target = Object.freeze(entries.map(entry => Object.freeze({ ...entry })))
+      const source = Object.freeze([Object.freeze({ ...existing }), Object.freeze({ ...added })])
+      const snapshot = structuredClone({ target, source })
+      const mergeDeep = new MergeDeep(log, jest.fn())
+      const result = mergeDeep.compareDeep(target, source)
+
+      expect({ target, source }).toEqual(snapshot)
+      expect(result.additions).toStrictEqual(entries.length ? [added] : source)
+      expect(result.additions).not.toBe(source)
+    })
+  })
+
   it('CompareDeep extensive test', () => {
     const target = YAML.load(`
 repository:
@@ -290,6 +403,7 @@ branches:
           }
         },
       modifications: {},
+      deletions: {},
       hasChanges: true
     }
     const ignorableFields = []
@@ -303,8 +417,7 @@ branches:
     )
     const merged = mergeDeep.compareDeep({}, source)
     console.log(`diffs ${JSON.stringify(merged, null, 2)}`)
-    expect(merged.additions).toEqual(expected.additions)
-    expect(merged.modifications.length).toEqual(expected.modifications.length)
+    expect(merged).toStrictEqual(expected)
 
     const overrideConfig = mergeDeep.mergeDeep({}, {}, source)
     const same = mergeDeep.compareDeep(overrideConfig, source)
@@ -809,6 +922,7 @@ entries:
         ]
       },
       modifications: {},
+      deletions: {},
       hasChanges: true
     }
     const ignorableFields = []
@@ -1065,7 +1179,7 @@ entries:
         pendinginvite: false,
         permission: 'admin'
       }],
-      modifications: {}
+      modifications: []
     }
 
     const ignorableFields = []
@@ -1080,13 +1194,13 @@ entries:
     const merged = mergeDeep.compareDeep(target, source)
     console.log(`diffs ${JSON.stringify(merged, null, 2)}`)
     expect(merged.deletions).toEqual(expected.deletions)
-    expect(merged.modifications.length).toEqual(expected.modifications.length)
+    expect(merged.modifications).toEqual(expected.modifications)
 
     const overrideConfig = mergeDeep.mergeDeep({}, target, source)
     const same = mergeDeep.compareDeep(overrideConfig, target)
-    expect(same.additions).toEqual({})
-    expect(same.modifications).toEqual({})
-    expect(same.modifications).toEqual({})
+    expect(same).toStrictEqual({
+      additions: [], modifications: [], deletions: [], hasChanges: false
+    })
   })
 
   it('Ruleset Compare Works when no changes', () => {

test/unit/lib/plugins/rulesets.test.js

@@ -177,8 +177,14 @@ describe('Rulesets', () => {
       const summary = flat.find(command => command.plugin === 'Rulesets' && command.action?.msg === 'Changes found')
 
       expect(flat.some(command => command.type === 'ERROR')).toBe(false)
-      expect(summary.action.additions['0']).toEqual(expect.objectContaining({ name: 'All branches' }))
-      expect(summary.action.deletions).toBeUndefined()
+      expect(summary.action).toEqual({
+        msg: 'Changes found',
+        additions: plugin.entries,
+        modifications: [],
+        deletions: []
+      })
+      expect(plugin.hasChanges).toBe(true)
+      expect(github.request).not.toHaveBeenCalled()
     })
   })
 

test/unit/lib/settings-results.test.js

@@ -1,5 +1,6 @@
 const Settings = require('../../../lib/settings')
 const Branches = require('../../../lib/plugins/branches')
+const Rulesets = require('../../../lib/plugins/rulesets')
 const NopCommand = require('../../../lib/nopcommand')
 const env = require('../../../lib/env')
 
@@ -145,6 +146,46 @@ describe('Settings result deduplication', () => {
     }
   })
 
+  it.each(['first', 'subsequent'])('preserves %s ruleset array additions through NOP, check-run and PR reporting', async stage => {
+    const added = { name: 'new-policy', target: 'branch', enforcement: 'active', rules: [{ type: 'deletion' }] }
+    const existing = stage === 'first'
+      ? []
+      : [{ id: 42, name: 'existing-policy', target: 'branch', enforcement: 'active', source_type: 'Repository', rules: [] }]
+    context.octokit.paginate = jest.fn().mockResolvedValue(existing)
+    context.octokit.request = jest.fn()
+    context.octokit.request.endpoint = Object.assign(
+      jest.fn((url, body) => ({ url, body })),
+      { merge: jest.fn((url, params) => ({ url, ...params })) }
+    )
+    const entries = [...existing, added]
+    const snapshot = structuredClone(entries)
+    const plugin = new Rulesets(true, context.octokit, repo, entries, context.log, [])
+    const results = (await plugin.sync()).flat()
+    const summary = results.find(row => row.action.msg === 'Changes found')
+
+    expect(summary.action).toEqual({
+      msg: 'Changes found', additions: [added], modifications: [], deletions: []
+    })
+    expect(plugin.hasChanges).toBe(true)
+    expect(results.map(row => row.action.msg)).toEqual(['Changes found', 'Create Ruleset'])
+    expect(results[1].body).toMatchObject(added)
+    expect(entries).toEqual(snapshot)
+    expect(context.octokit.request).not.toHaveBeenCalled()
+    settings.appendToResults(results)
+
+    await settings.handleResults()
+
+    expect(settings.results).toEqual(results)
+    const check = context.octokit.rest.checks.update.mock.calls[0][0]
+    const comment = context.octokit.rest.issues.createComment.mock.calls[0][0]
+    for (const output of [check.output.summary, comment.body]) {
+      expect(output).toContain('1 repo, 1 policy changed')
+      expect(output).toContain('`new-policy`')
+      expect(output).not.toContain('`0.')
+      expect(output).not.toContain('existing-policy')
+    }
+  })
+
   it('preserves suborg provenance before filtering unchanged org rulesets', async () => {
     const rulesets = [{ name: 'managed', enforcement: 'active' }]
     settings.config = { rulesets }