fix: normalize branch protection restrictions and preserve existing settings

Author: madkoo

lib/plugins/branches.js

@@ -81,7 +81,7 @@ module.exports = class Branches extends ErrorStash {
                   resArray.push(new NopCommand(this.constructor.name, this.repo, null, results))
                 }
 
-                Object.assign(params, requiredBranchProtectionDefaults, branch.protection, { headers: previewHeaders })
+                Object.assign(params, requiredBranchProtectionDefaults, this.reformatAndReturnBranchProtection(structuredClone(result.data)), branch.protection, { headers: previewHeaders })
 
                 if (this.nop) {
                   resArray.push(new NopCommand(this.constructor.name, this.repo, this.github.repos.updateBranchProtection.endpoint(params), 'Add Branch Protection'))
@@ -131,6 +131,18 @@ module.exports = class Branches extends ErrorStash {
       protection.required_linear_history = protection.required_linear_history && protection.required_linear_history.enabled
       protection.enforce_admins = protection.enforce_admins && protection.enforce_admins.enabled
       protection.required_signatures = protection.required_signatures && protection.required_signatures.enabled
+      if (protection.restrictions) {
+        delete protection.restrictions.url
+        protection.restrictions.users = Array.isArray(protection.restrictions.users)
+          ? protection.restrictions.users.map(user => user.login || user)
+          : []
+        protection.restrictions.teams = Array.isArray(protection.restrictions.teams)
+          ? protection.restrictions.teams.map(team => team.slug || team)
+          : []
+        protection.restrictions.apps = Array.isArray(protection.restrictions.apps)
+          ? protection.restrictions.apps.map(app => app.slug || app)
+          : []
+      }
       if (protection.required_pull_request_reviews && !protection.required_pull_request_reviews.bypass_pull_request_allowances) {
         protection.required_pull_request_reviews.bypass_pull_request_allowances = { apps: [], teams: [], users: [] }
       }

test/unit/lib/plugins/branches.test.js

@@ -181,20 +181,19 @@ describe('Branches', () => {
         )
 
         return plugin.sync().then(() => {
-          expect(github.repos.updateBranchProtection).toHaveBeenCalledWith({
+          expect(github.repos.updateBranchProtection).toHaveBeenCalledWith(expect.objectContaining({
             owner: 'bkeepers',
             repo: 'test',
             branch: 'main',
             required_status_checks: {
               strict: true,
               contexts: []
             },
-            // When override processing clears {{EXTERNALLY_DEFINED}} contexts,
-            // enforce_admins defaults to null since config doesn't specify it
-            enforce_admins: null,
+            // Existing enforce_admins should be preserved from GitHub
+            enforce_admins: false,
             restrictions: null,
             headers: { accept: 'application/vnd.github.hellcat-preview+json,application/vnd.github.luke-cage-preview+json,application/vnd.github.zzzax-preview+json' }
-          })
+          }))
         })
       })
     })
@@ -249,17 +248,44 @@ describe('Branches', () => {
           )
         })
       })
+
+      it('normalizes restrictions and defaults missing arrays when preserving from GitHub', () => {
+        github.repos.getBranchProtection = jest.fn().mockResolvedValue({
+          data: {
+            enforce_admins: { enabled: true },
+            restrictions: {
+              url: 'https://api.github.com/...',
+              users: [{ login: 'user1' }]
+            }
+          }
+        })
+
+        const plugin = configure([{
+          name: 'main',
+          protection: {
+            enforce_admins: false
+          }
+        }])
+
+        return plugin.sync().then(() => {
+          const payload = github.repos.updateBranchProtection.mock.calls[0][0]
+          expect(payload.restrictions).toEqual({
+            users: ['user1'],
+            teams: [],
+            apps: []
+          })
+          expect(payload.restrictions.url).toBeUndefined()
+        })
+      })
     })
 
     describe('when {{EXTERNALLY_DEFINED}} is present in "required_status_checks" and status checks exist in GitHub', () => {
       it('it retains the status checks from GitHub', () => {
         github.repos.getBranchProtection = jest.fn().mockResolvedValue({
           data: {
             enforce_admins: { enabled: false },
-            protection: {
-              required_status_checks: {
-                contexts: ['check-1', 'check-2']
-              }
+            required_status_checks: {
+              contexts: ['check-1', 'check-2']
             }
           }
         })
@@ -276,18 +302,18 @@ describe('Branches', () => {
         )
 
         return plugin.sync().then(() => {
-          expect(github.repos.updateBranchProtection).toHaveBeenCalledWith({
+          expect(github.repos.updateBranchProtection).toHaveBeenCalledWith(expect.objectContaining({
             owner: 'bkeepers',
             repo: 'test',
             branch: 'main',
             required_status_checks: {
               strict: true,
               contexts: ['check-1', 'check-2']
             },
-            enforce_admins: null,
+            enforce_admins: false,
             restrictions: null,
             headers: { accept: 'application/vnd.github.hellcat-preview+json,application/vnd.github.luke-cage-preview+json,application/vnd.github.zzzax-preview+json' }
-          })
+          }))
         })
       })
     })