fix(mergedeep): handle null actor_id in bypass actor comparison

docs/sample-settings/settings.yml

@@ -250,7 +250,9 @@ rulesets:
         #  - pull_request
         bypass_mode: pull_request
 
-      - actor_id: 1
+      # actor_id must be null for OrganizationAdmin: GitHub ignores the id for
+      # this actor type and returns null, so any other value never converges
+      - actor_id: null
         actor_type: OrganizationAdmin
         bypass_mode: always
 

lib/mergeDeep.js

@@ -2,7 +2,8 @@ const mergeBy = require('./mergeArrayBy')
 const DeploymentConfig = require('./deploymentConfig')
 
 const NAME_FIELDS = ['name', 'username', 'actor_id', 'login', 'type', 'key_prefix', 'context']
-const NAME_USERNAME_PROPERTY = item => NAME_FIELDS.find(prop => Object.prototype.hasOwnProperty.call(item, prop))
+const IDENTITY_FIELDS = ['name', 'username', 'actor_id', 'actor_type', 'login', 'type', 'key_prefix', 'context']
+const NAME_USERNAME_PROPERTY = item => IDENTITY_FIELDS.find(prop => Object.prototype.hasOwnProperty.call(item, prop) && item[prop] != null)
 const GET_NAME_USERNAME_PROPERTY = item => { if (NAME_USERNAME_PROPERTY(item)) return item[NAME_USERNAME_PROPERTY(item)] }
 
 class MergeDeep {
@@ -92,8 +93,8 @@ class MergeDeep {
     // So any property in the target that is not in the source is not treated as a deletion
     for (const key in source) {
       // Skip prototype pollution vectors
-      if (key === "__proto__" || key === "constructor") {
-        continue;
+      if (key === '__proto__' || key === 'constructor') {
+        continue
       }
       // Logic specific for Github
       // API response includes urls for resources, or other ignorable fields; we can ignore them

test/unit/lib/mergeDeep.test.js

@@ -1882,4 +1882,44 @@ branches:
     expect(same.additions).toEqual({})
     expect(same.modifications).toEqual({})
   })
+
+  describe('CompareDeep bypass actors with a null actor_id (OrganizationAdmin, DeployKey)', () => {
+    // GitHub ignores actor_id for these actor types and always returns
+    // actor_id: null, so the config should carry null and still diff correctly.
+    const mergeDeep = new MergeDeep(log, jest.fn(), [])
+    const orgAdmin = { actor_id: null, actor_type: 'OrganizationAdmin', bypass_mode: 'pull_request' }
+    const deployKey = { actor_id: null, actor_type: 'DeployKey', bypass_mode: 'always' }
+    const app = { actor_id: 210920, actor_type: 'Integration', bypass_mode: 'always' }
+
+    it('deploys a config-only actor from an empty ruleset', () => {
+      const result = mergeDeep.compareDeep({ bypass_actors: [] }, { bypass_actors: [orgAdmin] })
+      expect(result.hasChanges).toEqual(true)
+      expect(result.additions.bypass_actors).toEqual([orgAdmin])
+    })
+
+    it('deploys a config-only actor alongside an existing app actor', () => {
+      const result = mergeDeep.compareDeep({ bypass_actors: [app] }, { bypass_actors: [app, orgAdmin] })
+      expect(result.hasChanges).toEqual(true)
+      expect(result.additions.bypass_actors).toEqual([orgAdmin])
+    })
+
+    it('converges once the actor is live', () => {
+      const result = mergeDeep.compareDeep({ bypass_actors: [orgAdmin] }, { bypass_actors: [orgAdmin] })
+      expect(result.hasChanges).toEqual(false)
+    })
+
+    it('converges with mixed actors and still identifies real ids by actor_id', () => {
+      const result = mergeDeep.compareDeep({ bypass_actors: [app, orgAdmin] }, { bypass_actors: [app, orgAdmin] })
+      expect(result.hasChanges).toEqual(false)
+    })
+
+    it('handles a DeployKey actor the same way, distinct from other null-id actors', () => {
+      const deploys = mergeDeep.compareDeep({ bypass_actors: [orgAdmin] }, { bypass_actors: [orgAdmin, deployKey] })
+      expect(deploys.hasChanges).toEqual(true)
+      expect(deploys.additions.bypass_actors).toEqual([deployKey])
+
+      const converged = mergeDeep.compareDeep({ bypass_actors: [orgAdmin, deployKey] }, { bypass_actors: [orgAdmin, deployKey] })
+      expect(converged.hasChanges).toEqual(false)
+    })
+  })
 })