From e0d447aa6a5ea576e61e23370867ead1b1bbb250 Mon Sep 17 00:00:00 2001
From: Yadhav Jayaraman <57544838+decyjphr@users.noreply.github.com>
Date: Mon, 28 Sep 2026 13:59:07 -0400
Subject: [PATCH] fix(mergedeep): preserve top-level array diff shapes

Adapt PR #1030 to the evolved comparison implementation. Keep array diff buckets consistent across empty and populated targets, retain deletions on early returns, and skip unsafe object keys. Cover exact result shapes and first/subsequent ruleset dry-run reporting.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
---
 lib/mergeDeep.js                       |  42 ++++----
 test/unit/lib/mergeDeep.test.js        | 128 +++++++++++++++++++++++--
 test/unit/lib/plugins/rulesets.test.js |  10 +-
 test/unit/lib/settings-results.test.js |  41 ++++++++
 4 files changed, 189 insertions(+), 32 deletions(-)

diff --git a/lib/mergeDeep.js b/lib/mergeDeep.js
index 32d5e972c..9cb82437d 100644
--- a/lib/mergeDeep.js
+++ b/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) })
   }
diff --git a/test/unit/lib/mergeDeep.test.js b/test/unit/lib/mergeDeep.test.js
index 497dcc964..f3446ce0f 100644
--- a/test/unit/lib/mergeDeep.test.js
+++ b/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', () => {
diff --git a/test/unit/lib/plugins/rulesets.test.js b/test/unit/lib/plugins/rulesets.test.js
index 5c172825c..ccc92489f 100644
--- a/test/unit/lib/plugins/rulesets.test.js
+++ b/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()
     })
   })
 
diff --git a/test/unit/lib/settings-results.test.js b/test/unit/lib/settings-results.test.js
index 868b4dade..42985fe23 100644
--- a/test/unit/lib/settings-results.test.js
+++ b/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 }