Fix external group handling, mergeDeep crash, and config validator gap

lib/mergeDeep.js

@@ -329,13 +329,25 @@ class MergeDeep {
 
       if (!this.isEmpty(additions)) {
         for (const key in lastAddition) {
-          if (!lastModification[key]) {
-            lastModification[key] = Array.isArray(lastAddition[key]) ? [] : {}
-          }
-          if (!Array.isArray(lastAddition[key])) {
-            Object.assign(lastModification[key], lastAddition[key])
+          const addedValue = lastAddition[key]
+          if (Array.isArray(addedValue)) {
+            if (!Array.isArray(lastModification[key])) {
+              lastModification[key] = []
+            }
+            lastModification[key].push(...addedValue)
+          } else if (this.isObjectNotArray(addedValue)) {
+            if (!this.isObjectNotArray(lastModification[key])) {
+              lastModification[key] = {}
+            }
+            Object.assign(lastModification[key], addedValue)
           } else {
-            lastModification[key].push(...lastAddition[key])
+            // Primitive value (e.g. the `name`/`permission` identifying
+            // attribute added by addIdentifyingAttribute). Object.assign
+            // would box a primitive string target and throw
+            // "Cannot assign to read only property '0' of object
+            // '[object String]'" since string indices are read-only, so
+            // just overwrite directly instead of merging.
+            lastModification[key] = addedValue
           }
         }
         additions.length = 0

lib/plugins/teams.js

@@ -188,7 +188,14 @@ module.exports = class Teams extends Diffable {
         )
         const byName = new Map()
         for (const g of groups) {
-          if (g && g.group_name) byName.set(g.group_name, g.group_id)
+          // Keys are lower-cased so lookups are case-insensitive, matching
+          // the comparison used by the SCIM-sync workflow's
+          // wait-for-scim-sync.py (which lower-cases both sides). Without
+          // this, a yaml `external_group` value that differs only in case
+          // from the IdP-provisioned display name would be reported as
+          // "not found" even though the group exists and the CI gate
+          // considers it synced.
+          if (g && g.group_name) byName.set(g.group_name.toLowerCase(), g.group_id)
         }
         this.log.debug(`Loaded ${byName.size} external group(s) for org ${org}: ${JSON.stringify(Array.from(byName.keys()))}`)
         cache.set(org, byName)
@@ -198,7 +205,7 @@ module.exports = class Teams extends Diffable {
         cache.set(org, new Map())
       }
     }
-    const id = cache.get(org).get(groupName)
+    const id = cache.get(org).get(groupName.toLowerCase())
     if (id === undefined) {
       return null
     }
@@ -217,13 +224,15 @@ module.exports = class Teams extends Diffable {
 
     const groupId = await this.resolveExternalGroupId(groupName)
     if (groupId === null) {
-      const msg = `External group '${groupName}' not found for org ${this.repo.owner} (team '${attrs.name}').`
-      // logError: feeds the synchronous-run end-of-run errors summary.
-      this.logError(msg)
-      // For PR dry-run / nop mode, also surface the failure in the check_run
-      // output -- which is built from results entries with type === 'ERROR'.
+      const msg = `External group '${groupName}' not found for org ${this.repo.owner} (team '${attrs.name}'). This is expected if the team/group is newly added and has not finished SCIM-provisioning yet.`
+      // Non-fatal: a brand-new team's external group commonly doesn't exist
+      // yet until the Azure AD -> GitHub SCIM cycle completes, so this is
+      // logged as a warning (not logError) to avoid failing the whole sync.
+      this.log.warn(msg)
+      // For PR dry-run / nop mode, surface it as a WARNING (not ERROR) in the
+      // check_run output, so it's visible but doesn't mark the run as failed.
       if (this.nop && Array.isArray(nopCommands)) {
-        nopCommands.push(new NopCommand(this.constructor.name, this.repo, null, msg, 'ERROR'))
+        nopCommands.push(new NopCommand(this.constructor.name, this.repo, null, msg, 'WARNING'))
       }
       return
     }

lib/settings.js

@@ -162,6 +162,52 @@ function filterActionByChangedNames (action, changedNames) {
   return filtered
 }
 
+// ---------------------------------------------------------------------------
+// Centralized ruleset bypass_actors helpers
+// ---------------------------------------------------------------------------
+
+// Builds a de-duplication key for a bypass actor entry, keyed on actor_type
+// plus either actor_id or name (whichever is present).
+function bypassActorKey (actor) {
+  const actorType = actor.actor_type || 'unknown'
+  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
+// 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)
+// never mutates a shared object across multiple rulesets/repos, which sync
+// concurrently (see Settings#updateAll).
+function mergeBypassActors (existing, centralized) {
+  if (!Array.isArray(centralized) || centralized.length === 0) return existing
+  const merged = new Map()
+  for (const actor of (existing || [])) merged.set(bypassActorKey(actor), { ...actor })
+  for (const actor of centralized) merged.set(bypassActorKey(actor), { ...actor })
+  return Array.from(merged.values())
+}
+
+// Returns a copy of `rulesetEntries` with centrally-declared bypass actors
+// merged into every entry's bypass_actors. This runs before the Rulesets
+// plugin's own current-vs-desired diffing (Diffable.sync), so a ruleset only
+// results in an actual GitHub API call when its resulting bypass_actors
+// differ from what's already applied on GitHub — i.e. new or updated
+// rulesets, or ones missing the centralized actors. Unaffected, unchanged
+// rulesets are left as no-ops by the normal diff, with no special-casing
+// needed here.
+function applyCentralizedBypassActors (rulesetEntries, centralizedBypassActors) {
+  if (!Array.isArray(rulesetEntries) || !Array.isArray(centralizedBypassActors) || centralizedBypassActors.length === 0) {
+    return rulesetEntries
+  }
+  return rulesetEntries.map(ruleset => {
+    if (!ruleset || typeof ruleset !== 'object') return ruleset
+    return { ...ruleset, bypass_actors: mergeBypassActors(ruleset.bypass_actors, centralizedBypassActors) }
+  })
+}
+
 // ---------------------------------------------------------------------------
 // NOP change-rendering helpers (collapsible, field-level diff summaries)
 // ---------------------------------------------------------------------------
@@ -1008,7 +1054,7 @@ class Settings {
     if (this.baseConfig) {
       this.log.debug('Filtering NOP results using base config comparison')
       this.results = this.results.filter(res => {
-        if (!res || res.type === 'ERROR') return true
+        if (!res || res.type === 'ERROR' || res.type === 'WARNING') return true
 
         if (res.type === 'INFO' && res.action?.msg && res.action?.additions === null && res.action?.deletions === null && res.action?.modifications === null) {
           return true
@@ -1059,6 +1105,10 @@ class Settings {
       reposProcessed: {},
       changes: {},
       errors: {},
+      // Non-fatal entries (type === 'WARNING'), e.g. an external_group that
+      // doesn't exist yet because SCIM provisioning hasn't completed. Keyed
+      // by repo. Unlike errors, these do not flip the check_run conclusion.
+      warnings: {},
       // Informational entries (type === 'INFO', all-null diff fields), e.g.
       // disable_plugins skip messages. Keyed by repo.
       infos: {}
@@ -1075,6 +1125,14 @@ class Settings {
             ? (res.action.msg || res.action.message)
             : `${res.action}`
           stats.errors[res.repo].push({ msg })
+        } else if (res.type === 'WARNING') {
+          if (!stats.warnings[res.repo]) {
+            stats.warnings[res.repo] = []
+          }
+          const msg = res.action && (res.action.msg || res.action.message)
+            ? (res.action.msg || res.action.message)
+            : `${res.action}`
+          stats.warnings[res.repo].push({ msg })
         } else if (res.action?.additions === null && res.action?.deletions === null && res.action?.modifications === null) {
           // No diff data — informational message (e.g. disable_plugins skip).
           if (res.action?.msg) {
@@ -1120,6 +1178,15 @@ class Settings {
           ).join('\n\n')
         }\n\n</details>`
 
+      const warningRepos = Object.keys(stats.warnings)
+      const warningSection = warningRepos.length === 0
+        ? ''
+        : `### Warnings\n<details>\n<summary>:warning: Warnings — ${warningRepos.length} ${pluralize(warningRepos.length, 'repo', 'repos')} affected</summary>\n\n${
+          warningRepos.map(repo =>
+            `**${repo}**:\n${stats.warnings[repo].map(w => `* ${w.msg}`).join('\n')}`
+          ).join('\n\n')
+        }\n\n</details>`
+
       // Preserve disable_plugins informational messages in the PR comment.
       const infoRepos = Object.keys(stats.infos)
       const infoSection = infoRepos.length === 0
@@ -1130,7 +1197,7 @@ class Settings {
           ).join('\n\n')
         }\n\n</details>`
 
-      const trailingSections = infoSection ? [errorSection, infoSection] : [errorSection]
+      const trailingSections = [errorSection, warningSection, infoSection].filter(Boolean)
       const bodySections = stats.changeSections.length === 0
         ? ['_No changes to apply._', ...trailingSections]
         : [...pluginSectionList, ...trailingSections]
@@ -1370,7 +1437,7 @@ class Settings {
     const stripMap = this.computeStripMap()
     const additiveSet = this.normalizeAdditivePlugins()
 
-    const rulesetsConfig = this.config.rulesets
+    const rulesetsConfig = applyCentralizedBypassActors(this.config.rulesets, this.config.centralized_ruleset_bypass_actors)
     if (rulesetsConfig) {
       if (this.isPluginDisabledAnywhere(stripMap, 'rulesets')) {
         this.log.debug("disable_plugins: skipping org-level 'rulesets' plugin")
@@ -1795,9 +1862,12 @@ class Settings {
         if (section in Settings.PLUGINS) {
           this.log.debug(`Found section ${section} in the config. Creating plugin...`)
           const Plugin = Settings.PLUGINS[section]
+          const pluginConfig = section === 'rulesets'
+            ? applyCentralizedBypassActors(config, this.config.centralized_ruleset_bypass_actors)
+            : config
           // Include sectionName as 3rd element so callers can thread the
           // additive_plugins flag without re-deriving the plugin key.
-          childPlugins.push([Plugin, config, section])
+          childPlugins.push([Plugin, pluginConfig, section])
         }
       }
     }

test/unit/lib/mergeDeep.test.js

@@ -2027,4 +2027,47 @@ branches:
     expect(same.additions).toEqual({})
     expect(same.modifications).toEqual({})
   })
+
+  // Regression test for: TypeError: Cannot assign to read only property '0' of
+  // object '[object String]'. This was thrown by compareDeepIfVisited when a
+  // team entry had BOTH a changed primitive field (e.g. `permission`, which
+  // becomes a modification) AND a field the target lacks entirely (e.g.
+  // `external_group`, which becomes an addition). addIdentifyingAttribute adds
+  // the `name` identifying value as a plain string to both the addition and
+  // the modification containers, and the old merge logic then tried to
+  // Object.assign(modificationNameString, additionNameString) -- boxing a
+  // primitive string as the assignment target throws because string indices
+  // are read-only.
+  it('CompareDeep does not throw when a team has both a changed field and a new external_group field', () => {
+    const target = [
+      { id: 1, name: 'Azure-Security-GHEC-Runners-Developers', slug: 'azure-security-ghec-runners-developers', permission: 'push', privacy: 'closed' }
+    ]
+    const source = [
+      { name: 'Azure-Security-GHEC-Runners-Developers', permission: 'admin', external_group: 'Azure-Security-GHEC-Runners-Developers', privacy: 'closed' }
+    ]
+
+    const ignorableFields = ['id', 'node_id', 'url']
+    const mockReturnGitHubContext = jest.fn().mockReturnValue({
+      request: () => {}
+    })
+    const mergeDeep = new MergeDeep(
+      log,
+      mockReturnGitHubContext,
+      ignorableFields
+    )
+
+    let merged
+    expect(() => {
+      merged = mergeDeep.compareDeep(target, source)
+    }).not.toThrow()
+
+    expect(merged.hasChanges).toBe(true)
+    expect(merged.modifications).toEqual([
+      {
+        permission: 'admin',
+        name: 'Azure-Security-GHEC-Runners-Developers',
+        external_group: 'Azure-Security-GHEC-Runners-Developers'
+      }
+    ])
+  })
 })

test/unit/lib/plugins/teams.test.js

@@ -15,7 +15,7 @@ describe('Teams', () => {
   const org = 'bkeepers'
 
   function configure (config) {
-    const log = { debug: jest.fn(), error: console.error }
+    const log = { debug: jest.fn(), error: console.error, warn: console.warn }
     const errors = []
     return new Teams(undefined, github, { owner: 'bkeepers', repo: 'test' }, config, log, errors)
   }
@@ -192,7 +192,7 @@ describe('Teams', () => {
       )
     })
 
-    it('logs an error and skips when the external group name is not found', async () => {
+    it('logs a warning (not an error) and skips when the external group name is not found', async () => {
       const plugin = configure([
         { name: unchangedTeamName, permission: 'push', external_group: 'Nonexistent Group' }
       ])
@@ -203,12 +203,12 @@ describe('Teams', () => {
         'PATCH /orgs/{org}/teams/{team_slug}/external-groups',
         expect.anything()
       )
-      // logError pushes onto the errors array
-      expect(plugin.errors.some(e => /Nonexistent Group/.test(JSON.stringify(e)))).toBe(true)
+      // Non-fatal: should not push onto the errors array
+      expect(plugin.errors.some(e => /Nonexistent Group/.test(JSON.stringify(e)))).toBe(false)
     })
 
-    it('in nop mode, emits an ERROR NopCommand when the external group is not found (so it appears in the PR check_run)', async () => {
-      const log = { debug: jest.fn(), error: console.error }
+    it('in nop mode, emits a WARNING NopCommand when the external group is not found (so it appears in the PR check_run without failing it)', async () => {
+      const log = { debug: jest.fn(), error: console.error, warn: console.warn }
       const errors = []
       const Teams = require('../../../../lib/plugins/teams')
       const plugin = new Teams(true, github, { owner: org, repo: 'test' }, [
@@ -218,8 +218,9 @@ describe('Teams', () => {
       const result = await plugin.sync()
 
       expect(Array.isArray(result)).toBe(true)
-      const errorCmd = result.find(c => c && c.type === 'ERROR' && /Nonexistent Group/.test(JSON.stringify(c)))
-      expect(errorCmd).toBeDefined()
+      const warningCmd = result.find(c => c && c.type === 'WARNING' && /Nonexistent Group/.test(JSON.stringify(c)))
+      expect(warningCmd).toBeDefined()
+      expect(result.some(c => c && c.type === 'ERROR')).toBe(false)
       expect(github.request).not.toHaveBeenCalledWith(
         'PATCH /orgs/{org}/teams/{team_slug}/external-groups',
         expect.anything()