From 95c3ad66ecad0638767b124536ec5415636453a8 Mon Sep 17 00:00:00 2001 From: Vishal Dawange Date: Thu, 9 Jul 2026 23:31:30 -0500 Subject: [PATCH 1/2] Fix external group handling, mergeDeep crash, and config validator gap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit teams.js: - resolveExternalGroupId() now matches external_group names   case-insensitively, preventing false "not found" errors caused by   casing differences vs. the SCIM-synced Azure AD group name. - syncExternalGroup() no longer treats a missing external group as a   fatal error. Newly-created teams whose SCIM group hasn't finished   provisioning yet now log a warning and emit a WARNING NopCommand   instead of an ERROR, so the PR check surfaces it without failing. settings.js: - handleResults() gains full support for the new WARNING NopCommand   type: tracked in stats.warnings, rendered in a dedicated "Warnings"   PR comment section, and excluded from the check_run failure   condition (only ERROR still flips the conclusion to failure). - childPluginsList(): fixed a validation gap where config entries   declared only at suborg/repo level (no matching org-level baseline   entry by name) were never passed to configvalidators/   overridevalidators. Only entries matching an org-baseline entry were   validated; new entries with no org-level counterpart silently   bypassed validation (e.g. an invalid team `permission` value would   not be rejected). Now validated against an empty baseConfig too. mergeDeep.js: - Fixed a crash in compareDeepIfVisited(): merging an addition into a   modification unconditionally called Object.assign, which threw   "Cannot assign to read only property '0' of object '[object   String]'" when both values were primitive strings (e.g. the `name`   identifying attribute added by addIdentifyingAttribute). This   happened whenever an array item had both a modified field (e.g.   team `permission`) and a field missing from the target object (e.g.   `external_group`). The merge now branches by value type — array:   push, object: Object.assign, primitive: direct overwrite. Tests: - teams.test.js updated to assert the WARNING (not ERROR) NopCommand   and warning log for the missing-external-group case. - mergeDeep.test.js: added a regression test reproducing the   permission-change + new-external_group crash scenario. --- lib/mergeDeep.js | 24 ++++++--- lib/plugins/teams.js | 25 ++++++--- lib/settings.js | 78 +++++++++++++++++++++++++++-- test/unit/lib/mergeDeep.test.js | 43 ++++++++++++++++ test/unit/lib/plugins/teams.test.js | 17 ++++--- 5 files changed, 161 insertions(+), 26 deletions(-) diff --git a/lib/mergeDeep.js b/lib/mergeDeep.js index 04b8370af..32d5e972c 100644 --- a/lib/mergeDeep.js +++ b/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 diff --git a/lib/plugins/teams.js b/lib/plugins/teams.js index 0d655363a..0df459a2f 100644 --- a/lib/plugins/teams.js +++ b/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 } diff --git a/lib/settings.js b/lib/settings.js index bc89e016c..1daafbf9b 100644 --- a/lib/settings.js +++ b/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` + const warningRepos = Object.keys(stats.warnings) + const warningSection = warningRepos.length === 0 + ? '' + : `### Warnings\n
\n:warning: Warnings — ${warningRepos.length} ${pluralize(warningRepos.length, 'repo', 'repos')} affected\n\n${ + warningRepos.map(repo => + `**${repo}**:\n${stats.warnings[repo].map(w => `* ${w.msg}`).join('\n')}` + ).join('\n\n') + }\n\n
` + // 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` - 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]) } } } diff --git a/test/unit/lib/mergeDeep.test.js b/test/unit/lib/mergeDeep.test.js index 0d6506781..497dcc964 100644 --- a/test/unit/lib/mergeDeep.test.js +++ b/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' + } + ]) + }) }) diff --git a/test/unit/lib/plugins/teams.test.js b/test/unit/lib/plugins/teams.test.js index a521e0ce3..8879611c3 100644 --- a/test/unit/lib/plugins/teams.test.js +++ b/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() From 3ddbe1182810a199434576c880c0de9506ba2ca5 Mon Sep 17 00:00:00 2001 From: Yadhav Jayaraman <57544838+decyjphr@users.noreply.github.com> Date: Fri, 10 Jul 2026 11:29:39 -0400 Subject: [PATCH 2/2] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- lib/settings.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/settings.js b/lib/settings.js index 1daafbf9b..6da75fd35 100644 --- a/lib/settings.js +++ b/lib/settings.js @@ -185,7 +185,7 @@ function bypassActorKey (actor) { 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 (existing || [])) merged.set(bypassActorKey(actor), { ...actor }) for (const actor of centralized) merged.set(bypassActorKey(actor), { ...actor }) return Array.from(merged.values()) }