fix(mergedeep): converge identifier-less bypass actors

README.md

@@ -363,6 +363,12 @@ Notes:
 > ⚠️ **Warning:**
 When `{{EXTERNALLY_DEFINED}}` is removed from an existing branch protection rule or ruleset configuration, the status checks in the existing rules in GitHub will revert to the checks that are defined in safe-settings. From this point onwards, all status checks configured through the GitHub UI will be reverted back to the safe-settings configuration.
 
+Entries in the organization-level `centralized_ruleset_bypass_actors` replace
+matching actors in individual rulesets. For `OrganizationAdmin` and `DeployKey`,
+identity is the actor type alone: GitHub ignores `actor_id`, so omitted, null,
+and concrete IDs all refer to the same actor. The centralized entry's
+`bypass_mode` takes precedence without adding a duplicate actor.
+
 #### Referencing ruleset bypass actors and reviewers by name
 
 Rulesets normally require numeric ids for `bypass_actors[].actor_id` and for the
@@ -972,6 +978,9 @@ node smoke-test.js --phase 1-3
 npm run smoke-test:phase -- 1,3,5
 node smoke-test.js --phase 1,3,5
 
+# Bypass actor apply/NOP convergence (Phase 1 creates the required test repo)
+node smoke-test.js --phase 1,19
+
 # Mix range + interactive
 npm run smoke-test:phase -- 1-3 interactive
 node smoke-test.js --phase 1-3 --interactive
@@ -997,6 +1006,7 @@ The smoke test runs the following phases:
 | **Phase 11** | Validates `additive_plugins` — verifies additive-mode plugin behaviour |
 | **Phase 12** | Tests `custom_properties` plugin |
 | **Phase 13** | Tests the `variables` plugin (create, update, remove variables) |
+| **Phase 19** | Tests ignored `OrganizationAdmin`/`DeployKey` IDs, order-independent NOP convergence, real bypass-mode/role-ID changes, and no redundant updates (requires Phase 1) |
 | **Teardown** | Shuts down safe-settings, deletes test repos, teams, custom roles, and rulesets |
 
 ### Output

docs/sample-settings/settings.yml

@@ -253,14 +253,17 @@ rulesets:
         # - Team
         # - Integration
         # - OrganizationAdmin
+        # - DeployKey
         actor_type: Team
         #  When the specified actor can bypass the ruleset. `pull_request`
         #  means that an actor can only bypass rules on pull requests.
         #  - always
         #  - pull_request
         bypass_mode: pull_request
 
-      - actor_id: 1
+      # GitHub ignores actor_id for OrganizationAdmin and DeployKey and returns
+      # null. Use null (or omit actor_id) for these actor types.
+      - actor_id: null
         actor_type: OrganizationAdmin
         bypass_mode: always
 

lib/mergeDeep.js

@@ -3,7 +3,8 @@ 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 GET_NAME_USERNAME_PROPERTY = item => { if (NAME_USERNAME_PROPERTY(item)) return item[NAME_USERNAME_PROPERTY(item)] }
+const ignoresActorId = item => (item.actor_type === 'OrganizationAdmin' || item.actor_type === 'DeployKey') &&
+  Object.prototype.hasOwnProperty.call(item, 'bypass_mode')
 
 // Fields within a rule's `parameters` that are managed/defaulted by the GitHub API.
 // They should not be treated as user-driven deletions when omitted from config.
@@ -24,23 +25,24 @@ const stableStringify = value => {
   return JSON.stringify(value)
 }
 
-// Compute the identity value for an array element so the same logical item in
-// `source` (config) and `target` (GitHub API) can be paired during comparison.
-// Returns the raw identifying value (so a bare string shorthand like 'developers'
-// still matches an object like { name: 'developers' }). Special-cases bypass
-// actors: GitHub returns `actor_id: null` for role-based actor types such as
-// `OrganizationAdmin`, so we key those on `actor_type` to avoid spurious
-// add/delete churn when config supplies an explicit id.
-const getItemIdentity = item => {
+// Use the same identifying field for array pairing and displayed modifications.
+// GitHub ignores IDs for OrganizationAdmin and DeployKey, so key them by type.
+const getItemIdentityProperty = item => {
   if (!item || typeof item !== 'object' || Array.isArray(item)) return undefined
   if (Object.prototype.hasOwnProperty.call(item, 'actor_type') &&
       Object.prototype.hasOwnProperty.call(item, 'bypass_mode')) {
-    if (item.actor_id === null || item.actor_id === undefined || item.actor_type === 'OrganizationAdmin') {
-      return item.actor_type
+    if (item.actor_id === null || item.actor_id === undefined || ignoresActorId(item)) {
+      return 'actor_type'
     }
-    return item.actor_id
+    return 'actor_id'
   }
-  return GET_NAME_USERNAME_PROPERTY(item)
+  return NAME_USERNAME_PROPERTY(item)
+}
+
+const getItemIdentity = item => {
+  const property = getItemIdentityProperty(item)
+  // Keep raw values so string shorthands still match objects with a name.
+  return property === undefined ? undefined : item[property]
 }
 
 class MergeDeep {
@@ -140,6 +142,10 @@ class MergeDeep {
       if (key.indexOf('url') >= 0 || this.ignorableFields.indexOf(key) >= 0) {
         continue
       }
+      // These actor types have no meaningful ID, even when config supplies one.
+      if (key === 'actor_id' && ignoresActorId(source) && target.actor_type === source.actor_type) {
+        continue
+      }
       const sourceValue = source[key]
       const targetValue = target[key]
       if (targetValue === undefined) {
@@ -171,20 +177,13 @@ class MergeDeep {
         }
       } else { // The entry is a simple primitive
         if (targetValue !== sourceValue) {
-          // GitHub returns `actor_id: null` for role-based bypass actor types
-          // (e.g. OrganizationAdmin) regardless of the id supplied in config.
-          // Don't treat that placeholder mismatch as a modification.
-          if (key === 'actor_id' && (targetValue === null || sourceValue === null)) {
-            // treat as equal
-          } else {
-            // Note: source[key] cannot be undefined here since we are iterating on source keys
-            // so we don't need to check for that.
-            // The entries are different. It is an addition
-            modifications[key] = sourceValue
-            // retroactively add `name` or `username` to the modifications
-            // Since those are the only fields that can be used to identify the resource
-            this.addIdentifyingAttribute(source, key, modifications)
-          }
+          // Note: source[key] cannot be undefined here since we are iterating on source keys
+          // so we don't need to check for that.
+          // The entries are different. It is an addition
+          modifications[key] = sourceValue
+          // retroactively add `name` or `username` to the modifications
+          // Since those are the only fields that can be used to identify the resource
+          this.addIdentifyingAttribute(source, key, modifications)
         } else {
           // The entry is the same in both objects
         }
@@ -220,10 +219,10 @@ class MergeDeep {
   }
 
   addIdentifyingAttribute (source, key, containerObject) {
-    const id = NAME_USERNAME_PROPERTY(source)
+    const id = getItemIdentityProperty(source)
     if (id) {
       this.log.debug(`Adding name for ${key} ${source[key]}`)
-      containerObject[id] = GET_NAME_USERNAME_PROPERTY(source)
+      containerObject[id] = source[id]
     }
   }
 
@@ -351,7 +350,7 @@ class MergeDeep {
       // Add name attribute to the modifications to make it look better ; it won't be added otherwise as it would be the same
       if (!this.isEmpty(modifications[modifications.length - 1])) {
         if (visited[visitedId]) {
-          const displayProp = NAME_USERNAME_PROPERTY(a)
+          const displayProp = getItemIdentityProperty(a)
           if (displayProp) {
             modifications[modifications.length - 1][displayProp] = a[displayProp]
           }

lib/settings.js

@@ -169,16 +169,17 @@ function filterActionByChangedNames (action, changedNames) {
 // ---------------------------------------------------------------------------
 
 // Builds a de-duplication key for a bypass actor entry, keyed on actor_type
-// plus either actor_id or name (whichever is present).
+// alone for identifier-less actors, otherwise plus actor_id or name.
 function bypassActorKey (actor) {
   const actorType = actor.actor_type || 'unknown'
+  if (actorType === 'OrganizationAdmin' || actorType === 'DeployKey') return `actor_type:${actorType}`
   if (actor.actor_id !== undefined && actor.actor_id !== null) return `actor_id:${actorType}:${actor.actor_id}`
   if (actor.name) return `name:${actorType}:${actor.name}`
   return JSON.stringify(actor)
 }
 
 // Merges centrally-declared bypass actors into a ruleset's existing
-// bypass_actors, de-duplicating by (actor_type, actor_id|name). Centrally
+// bypass_actors, de-duplicating by actor type and any meaningful ID or name. Centrally
 // declared actors take precedence over a repo/suborg-declared entry with the
 // same key (e.g. to update bypass_mode). Every entry is shallow-cloned so
 // the Rulesets plugin's in-place name->id resolution (resolveBypassActor)

smoke-test.js

@@ -3185,6 +3185,91 @@ async function phase18TeamIncludeExclude () {
   await deleteBranch(ORG, ADMIN_REPO, branch)
 }
 
+async function phase19BypassActorConvergence () {
+  logPhase('Phase 19: Ruleset ignored bypass actor IDs and convergence')
+  const defaultBranch = await getDefaultBranch()
+  const name = 'smoke-null-bypass-actors'
+  const actor = (actorType, actorId, bypassMode = 'always') => ({
+    actor_type: actorType,
+    ...(actorId === undefined ? {} : { actor_id: actorId }),
+    bypass_mode: bypassMode
+  })
+  const steps = [
+    ['19a', 'create', [
+      actor('OrganizationAdmin', 1), actor('DeployKey', 1), actor('RepositoryRole', 4), actor('RepositoryRole', 5)
+    ], true],
+    ['19b', 'reorder and omit ignored ids', [
+      actor('RepositoryRole', 5), actor('DeployKey', null), actor('OrganizationAdmin', undefined), actor('RepositoryRole', 4)
+    ], false],
+    ['19c', 'change bypass mode and remove a real role id', [
+      actor('OrganizationAdmin', null, 'pull_request'), actor('DeployKey', null), actor('RepositoryRole', 5)
+    ], true],
+    ['19d', 'converge with concrete ignored ids', [
+      actor('RepositoryRole', 5), actor('DeployKey', 9), actor('OrganizationAdmin', 8, 'pull_request')
+    ], false]
+  ]
+  const sortActors = actors => actors.slice().sort((a, b) =>
+    `${a.actor_type}:${a.actor_id}`.localeCompare(`${b.actor_type}:${b.actor_id}`))
+  let previous
+
+  for (const [id, description, actors, changes] of steps) {
+    const branch = `smoke-test-phase${id}`
+    const attrs = {
+      name,
+      target: 'branch',
+      enforcement: 'disabled',
+      conditions: { ref_name: { include: ['~DEFAULT_BRANCH'], exclude: [] } },
+      bypass_actors: actors,
+      rules: [{ type: 'deletion' }]
+    }
+    // Keep unrelated property and pull-request-rule defaults out of NOP checks.
+    const configYaml = require('js-yaml').dump({ repository: { name: 'test' }, rulesets: [attrs] })
+    await deleteBranch(ORG, ADMIN_REPO, branch)
+    await createBranch(ORG, ADMIN_REPO, branch)
+    await createOrUpdateFile(ORG, ADMIN_REPO, `${CONFIG_PATH}/repos/test.yml`, configYaml, branch, `${id}: ${description}`)
+    const pr = await createPR(ORG, ADMIN_REPO, `${id}: bypass actors ${description}`, branch, defaultBranch)
+
+    await sleep(WEBHOOK_SETTLE_MS)
+    const checkRun = await waitForCheckRun(ORG, ADMIN_REPO, pr.head.sha)
+    if (!assert(checkRun && checkRun.conclusion === 'success', `${id}: NOP check completed successfully`)) {
+      throw new Error(`${id}: cannot apply without a successful NOP check`)
+    }
+    const summary = checkRun.output && checkRun.output.summary
+    if (changes) {
+      assert(summary && summary.includes(name), `${id}: NOP reports the changed bypass actor policy`)
+    } else {
+      assert(summary && /No changes to apply/i.test(summary), `${id}: NOP reports no changes after ignored-id/reordering changes`)
+    }
+
+    if (!await safeMerge(ORG, ADMIN_REPO, pr.number)) return
+    await sleep(WEBHOOK_SETTLE_MS)
+    const expectedActors = sortActors(actors.map(entry => actor(
+      entry.actor_type,
+      entry.actor_type === 'OrganizationAdmin' || entry.actor_type === 'DeployKey' ? null : entry.actor_id,
+      entry.bypass_mode
+    )))
+    const details = await poll(async () => {
+      const ruleset = await getRepoRuleset(ORG, 'test', name)
+      if (!ruleset) return null
+      const live = await getRepoRulesetDetails(ORG, 'test', ruleset.id)
+      if (!live || !Array.isArray(live.bypass_actors)) return null
+      const actualActors = sortActors(live.bypass_actors.map(entry => actor(entry.actor_type, entry.actor_id, entry.bypass_mode)))
+      return JSON.stringify(actualActors) === JSON.stringify(expectedActors) ? live : null
+    }, { desc: `${id}: exact bypass actors, modes and real role ids to be applied` })
+    if (!assert(details !== null, `${id}: exact actor set applied with null ignored IDs and expected real IDs`)) {
+      throw new Error(`${id}: bypass actor state did not converge`)
+    }
+    if (previous) {
+      assert(details.id === previous.id, `${id}: existing ruleset updated in place`)
+      if (!changes) {
+        assert(typeof details.updated_at === 'string' && details.updated_at === previous.updated_at, `${id}: second sync did not rewrite the ruleset`)
+      }
+    }
+    previous = details
+    await deleteBranch(ORG, ADMIN_REPO, branch)
+  }
+}
+
 async function main () {
   const { App } = await import('octokit')
   const app = new App({ appId: APP_ID, privateKey: PRIVATE_KEY })
@@ -3253,7 +3338,8 @@ async function main () {
       ['Phase 15: Ruleset array drift', phase15RulesetArrayDrift],
       ['Phase 16: Ruleset name/slug resolution', phase16RulesetNameResolution],
       ['Phase 17: App installation management', phase17AppInstallations],
-      ['Phase 18: Team include/exclude filters', phase18TeamIncludeExclude]
+      ['Phase 18: Team include/exclude filters', phase18TeamIncludeExclude],
+      ['Phase 19: Bypass actor convergence', phase19BypassActorConvergence]
     ]
 
     // When --phase is given, only run setup (phase 0) + the requested phase(s).

test/unit/lib/mergeDeep.test.js

@@ -4,6 +4,97 @@ const YAML = require('js-yaml')
 const log = require('pino')('test.log')
 
 describe('MergeDeep Test', () => {
+  describe('bypass actor comparison', () => {
+    const mergeDeep = new MergeDeep(log, jest.fn())
+    const actor = (actorType, actorId, bypassMode = 'always') => ({
+      actor_type: actorType,
+      ...(actorId === undefined ? {} : { actor_id: actorId }),
+      bypass_mode: bypassMode
+    })
+    const admin = actor('OrganizationAdmin', null)
+    const deployKey = actor('DeployKey', null)
+    const app = actor('Integration', 210920)
+
+    it.each([
+      ['empty ruleset', [], [admin], [admin]],
+      ['existing app', [app], [app, admin], [admin]],
+      ['distinct null-id actor', [admin], [admin, deployKey], [deployKey]]
+    ])('adds a null-id actor to %s', (_label, target, source, additions) => {
+      expect(mergeDeep.compareDeep({ bypass_actors: target }, { bypass_actors: source })).toStrictEqual({
+        additions: { bypass_actors: additions }, modifications: {}, deletions: {}, hasChanges: true
+      })
+    })
+
+    it.each([
+      [[admin], [admin]],
+      [[app, admin], [app, admin]],
+      [[admin, deployKey, app], [app, deployKey, admin]]
+    ])('converges with distinct null-id and real-id actors %p', (target, source) => {
+      expect(mergeDeep.compareDeep({ bypass_actors: target }, { bypass_actors: source })).toStrictEqual({
+        additions: {}, modifications: {}, deletions: {}, hasChanges: false
+      })
+    })
+
+    describe.each(['OrganizationAdmin', 'DeployKey'])('%s ignores actor_id', actorType => {
+      const ids = [undefined, null, 1, 2]
+      it.each(ids.flatMap(live => ids.map(config => [live, config])))('converges for live %p and configured %p without mutation', (liveId, configId) => {
+        const target = Object.freeze({ bypass_actors: Object.freeze([Object.freeze(actor(actorType, liveId))]) })
+        const source = Object.freeze({ bypass_actors: Object.freeze([Object.freeze(actor(actorType, configId))]) })
+        const before = structuredClone({ target, source })
+
+        expect(mergeDeep.compareDeep(target, source)).toStrictEqual({
+          additions: {}, modifications: {}, deletions: {}, hasChanges: false
+        })
+        expect({ target, source }).toEqual(before)
+      })
+
+      it('reports real bypass_mode changes with the actor type as identity', () => {
+        expect(mergeDeep.compareDeep(
+          { bypass_actors: [actor(actorType, null)] },
+          { bypass_actors: [actor(actorType, 1, 'pull_request')] }
+        )).toStrictEqual({
+          additions: {},
+          modifications: { bypass_actors: [{ actor_type: actorType, bypass_mode: 'pull_request' }] },
+          deletions: {},
+          hasChanges: true
+        })
+      })
+    })
+
+    it('deletes only the removed null-id actor', () => {
+      expect(mergeDeep.compareDeep(
+        { bypass_actors: [deployKey, admin, app] },
+        { bypass_actors: [app, actor('OrganizationAdmin', 1)] }
+      )).toStrictEqual({
+        additions: {}, modifications: {}, deletions: { bypass_actors: [deployKey] }, hasChanges: true
+      })
+    })
+
+    it.each(['Team', 'Integration', 'RepositoryRole', 'User'])('preserves real %s id changes and distinct actors of the same type', actorType => {
+      const unchanged = actor(actorType, 7)
+      const removed = actor(actorType, 42)
+      const added = actor(actorType, 99)
+      expect(mergeDeep.compareDeep(
+        { bypass_actors: [removed, unchanged, admin] },
+        { bypass_actors: [actor('OrganizationAdmin', 1), unchanged, added] }
+      )).toStrictEqual({
+        additions: { bypass_actors: [added] }, modifications: {}, deletions: { bypass_actors: [removed] }, hasChanges: true
+      })
+      expect(mergeDeep.compareDeep(actor(actorType, null), removed)).toStrictEqual({
+        additions: {}, modifications: { actor_id: 42 }, deletions: {}, hasChanges: true
+      })
+    })
+
+    it('preserves name precedence for non-bypass objects', () => {
+      expect(mergeDeep.compareDeep(
+        [{ name: 'policy', actor_id: 1, type: 'old' }],
+        [{ name: 'policy', actor_id: 2, type: 'new' }]
+      )).toStrictEqual({
+        additions: [], modifications: [{ name: 'policy', actor_id: 2, type: 'new' }], deletions: [], hasChanges: true
+      })
+    })
+  })
+
   describe('compareDeep result shape', () => {
     const existing = { name: 'existing', enforcement: 'active' }
     const added = { name: 'new', enforcement: 'active' }