From 0b45e69c73a4deafeac40610d23760a7bc71827e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomas=20Daba=C5=A1inskas?= Date: Fri, 24 Jul 2026 09:01:05 +0300 Subject: [PATCH 1/2] fix(mergedeep): handle null actor_id in bypass actor comparison GitHub ignores actor_id for certain bypass actor types (OrganizationAdmin, DeployKey) and always returns null. Previously, the merge logic would fail to converge because it couldn't properly identify these actors when comparing config to live state. Changes: - Add actor_type to IDENTITY_FIELDS for proper actor differentiation - Skip null-valued identity fields when finding the primary identifier - Update sample config to use null actor_id for OrganizationAdmin - Add test coverage for null actor_id scenarios --- docs/sample-settings/settings.yml | 4 +++- lib/mergeDeep.js | 7 ++++--- test/unit/lib/mergeDeep.test.js | 30 ++++++++++++++++++++++++++++++ 3 files changed, 37 insertions(+), 4 deletions(-) diff --git a/docs/sample-settings/settings.yml b/docs/sample-settings/settings.yml index 1ede6a079..9b6427a93 100644 --- a/docs/sample-settings/settings.yml +++ b/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 diff --git a/lib/mergeDeep.js b/lib/mergeDeep.js index ab278e5c2..c6b75bcbc 100644 --- a/lib/mergeDeep.js +++ b/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 diff --git a/test/unit/lib/mergeDeep.test.js b/test/unit/lib/mergeDeep.test.js index dd8bd60ba..c53bcbf30 100644 --- a/test/unit/lib/mergeDeep.test.js +++ b/test/unit/lib/mergeDeep.test.js @@ -1882,4 +1882,34 @@ 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 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) + }) + }) }) From 2e5ac4ade0095dae18d33fcc4517f40e8973a1b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomas=20Daba=C5=A1inskas?= Date: Fri, 24 Jul 2026 09:13:02 +0300 Subject: [PATCH 2/2] test(mergedeep): add test for null actor_id with different types Add test case to verify that bypass actors with null `actor_id` but different `actor_type` values (e.g., OrganizationAdmin vs DeployKey) are correctly distinguished during comparison. This ensures the fix for null actor_id comparison properly handles multiple actor types with null IDs. --- test/unit/lib/mergeDeep.test.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/unit/lib/mergeDeep.test.js b/test/unit/lib/mergeDeep.test.js index c53bcbf30..57f152b0a 100644 --- a/test/unit/lib/mergeDeep.test.js +++ b/test/unit/lib/mergeDeep.test.js @@ -1888,6 +1888,7 @@ branches: // 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', () => { @@ -1911,5 +1912,14 @@ branches: 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) + }) }) })