diff --git a/index.js b/index.js index 12aae7c7..4a4f0fe8 100644 --- a/index.js +++ b/index.js @@ -19,7 +19,7 @@ let deploymentConfig module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) => { let appSlug = 'safe-settings' - async function syncAllSettings (nop, context, repo = context.repo(), ref) { + async function syncAllSettings (nop, context, repo = context.repo(), ref, baseRef, changedFiles = {}) { try { deploymentConfig = await loadYamlFileSystem() robot.log.debug(`deploymentConfig is ${JSON.stringify(deploymentConfig)}`) @@ -27,8 +27,21 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) => const runtimeConfig = await configManager.loadGlobalSettingsYaml() const config = Object.assign({}, deploymentConfig, runtimeConfig) robot.log.debug(`config for ref ${ref} is ${JSON.stringify(config)}`) + + // Load base branch config for NOP filtering (only show PR-introduced changes) + let baseConfig = null + if (nop && baseRef) { + try { + const baseConfigManager = new ConfigManager(context, baseRef) + const baseRuntimeConfig = await baseConfigManager.loadGlobalSettingsYaml() + baseConfig = Object.assign({}, deploymentConfig, baseRuntimeConfig) + } catch (e) { + robot.log.debug(`Could not load base config for NOP filtering: ${e.message}`) + } + } + if (ref) { - return Settings.syncAll(nop, context, repo, config, ref) + return Settings.syncAll(nop, context, repo, config, ref, baseConfig, changedFiles) } else { return Settings.syncAll(nop, context, repo, config) } @@ -73,7 +86,7 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) => } } - async function syncSelectedSettings (nop, context, repos, subOrgs, ref) { + async function syncSelectedSettings (nop, context, repos, subOrgs, ref, baseRef) { try { deploymentConfig = await loadYamlFileSystem() robot.log.debug(`deploymentConfig is ${JSON.stringify(deploymentConfig)}`) @@ -81,7 +94,20 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) => const runtimeConfig = await configManager.loadGlobalSettingsYaml() const config = Object.assign({}, deploymentConfig, runtimeConfig) robot.log.debug(`config for ref ${ref} is ${JSON.stringify(config)}`) - return Settings.syncSelectedRepos(nop, context, repos, subOrgs, config, ref) + + // Load base branch config for NOP filtering + let baseConfig = null + if (nop && baseRef) { + try { + const baseConfigManager = new ConfigManager(context, baseRef) + const baseRuntimeConfig = await baseConfigManager.loadGlobalSettingsYaml() + baseConfig = Object.assign({}, deploymentConfig, baseRuntimeConfig) + } catch (e) { + robot.log.debug(`Could not load base config for NOP filtering: ${e.message}`) + } + } + + return Settings.syncSelectedRepos(nop, context, repos, subOrgs, config, ref, baseConfig) } catch (e) { if (nop) { let filename = env.SETTINGS_FILE_PATH @@ -585,17 +611,21 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) => const files = changes.data.map(f => { return f.filename }) const settingsModified = files.includes(Settings.FILE_PATH) + const repoChanges = getChangedRepoConfigName(files, context.repo().owner) + const subOrgChanges = getChangedSubOrgConfigName(files) if (settingsModified) { robot.log.debug(`Changes in '${Settings.FILE_PATH}' detected, doing a full synch...`) - return syncAllSettings(true, context, context.repo(), pull_request.head.ref) + const baseRef = pull_request.base.ref || repository.default_branch + return syncAllSettings(true, context, context.repo(), pull_request.head.ref, baseRef, { + repos: repoChanges, + subOrgs: subOrgChanges + }) } - const repoChanges = getChangedRepoConfigName(files, context.repo().owner) - const subOrgChanges = getChangedSubOrgConfigName(files) - if (repoChanges.length > 0 || subOrgChanges.length > 0) { - return syncSelectedSettings(true, context, repoChanges, subOrgChanges, pull_request.head.ref) + const baseRef = pull_request.base.ref || repository.default_branch + return syncSelectedSettings(true, context, repoChanges, subOrgChanges, pull_request.head.ref, baseRef) } // if no safe-settings changes detected, send a success to the check run diff --git a/lib/commentmessage.js b/lib/commentmessage.js index b54f81bb..22ee4417 100644 --- a/lib/commentmessage.js +++ b/lib/commentmessage.js @@ -1,31 +1,32 @@ -module.exports = `* Run on: \` <%= new Date() %> \` +module.exports = `* Run on: \`<%= new Date() %>\` -* Number of repos that were considered: \`<%= Object.keys(it.reposProcessed).length %> \` +* Number of repos that were considered: \`<%= Object.keys(it.reposProcessed).length %>\` +* Number of repos affected: \`<%= it.reposAffected || 0 %>\` ### Breakdown of changes -| Repo <% Object.keys(it.changes).forEach(plugin => { %> | <%= plugin %> settings <% }) %> | -| -- <% Object.keys(it.changes).forEach(plugin => { -%> | -- <% }) %> -| -<% Object.keys(it.reposProcessed).forEach( repo => { -%> -| <%= repo -%> - <%- Object.keys(it.changes).forEach(plugin => { -%> - <%_ if (it.changes[plugin][repo]) { -%> | :hand: <% } else { %> | :grey_exclamation: <% } -%> - <%_ }) -%> | -<% }) -%> - -:hand: -> Changes to be applied to the GitHub repository. -:grey_exclamation: -> nothing to be changed in that particular GitHub repository. +<% if (!it.changeSections || it.changeSections.length === 0) { %> -### Breakdown of errors +No changes to apply. +<% } else { %> +<%~ it.checkRunDetails %> +<% } %> + +### Breakdown of errors <% if (Object.keys(it.errors).length === 0) { %> + \`None\` <% } else { %> - <% Object.keys(it.errors).forEach(repo => { %> - <%_= repo %>: - <% it.errors[repo].forEach(plugin => { %> - * <%= plugin.msg %> - <% }) %> - <% }) %> +
+Errors by repo + +<% Object.keys(it.errors).forEach(function(repo) { %> +**<%= repo %>** + +<% it.errors[repo].forEach(function(err) { %>* <%= err.msg %> +<% }) %> +<% }) %> + +
<% } %>` diff --git a/lib/plugins/custom_properties.js b/lib/plugins/custom_properties.js index 35f0144d..0dc3c340 100644 --- a/lib/plugins/custom_properties.js +++ b/lib/plugins/custom_properties.js @@ -12,24 +12,7 @@ module.exports = class CustomProperties extends Diffable { // Force all names to lowercase to avoid comparison issues. normalizeEntries () { - this.entries = this.entries.reduce((normalizedEntries, entry) => { - if (!entry || typeof entry !== 'object') { - return normalizedEntries - } - - const entryName = entry.name || entry.property_name - - if (typeof entryName !== 'string') { - return normalizedEntries - } - - normalizedEntries.push({ - name: entryName.toLowerCase(), - value: entry.value - }) - - return normalizedEntries - }, []) + this.entries = this.normalize(this.entries) } async find () { @@ -52,24 +35,16 @@ module.exports = class CustomProperties extends Diffable { // Force all names to lowercase to avoid comparison issues. normalize (properties) { - return properties.reduce((normalizedProperties, property) => { - if (!property || typeof property !== 'object') { - return normalizedProperties - } - - const propertyName = property.property_name || property.name - - if (typeof propertyName !== 'string') { - return normalizedProperties - } - - normalizedProperties.push({ - name: propertyName.toLowerCase(), - value: property.value - }) - - return normalizedProperties - }, []) + return properties + .map(({ property_name: propertyName, name, value }) => ({ + name: propertyName ?? name, + value + })) + .filter(({ name }) => name != null) + .map(({ name, value }) => ({ + name: name.toLowerCase(), + value + })) } comparator (existing, attrs) { diff --git a/lib/settings.js b/lib/settings.js index 919e0185..920dc4bb 100644 --- a/lib/settings.js +++ b/lib/settings.js @@ -10,15 +10,486 @@ const env = require('./env') const CONFIG_PATH = env.CONFIG_PATH const eta = new Eta({ views: path.join(__dirname) }) const SCOPE = { ORG: 'org', REPO: 'repo' } // Determine if the setting is a org setting or repo setting +const COMMENT_LIMIT = 55536 const yaml = require('js-yaml') +function isDeepEmpty (value) { + if (value === null || value === undefined) return true + if (Array.isArray(value)) return value.length === 0 || value.every(isDeepEmpty) + if (typeof value === 'object') { + const keys = Object.keys(value) + return keys.length === 0 || keys.every(k => isDeepEmpty(value[k])) + } + return false +} + +function isEmptyChange (action) { + if (!action) return true + const { additions, deletions, modifications } = action + if (additions === null && deletions === null && modifications === null) return true + return isDeepEmpty(additions) && isDeepEmpty(deletions) && isDeepEmpty(modifications) +} + +/** + * Determines which named entries in an array-based config section actually changed + * between the base branch and the PR branch. Returns a Set of entry names that differ. + */ +function getChangedEntryNames (baseEntries, prEntries) { + const changed = new Set() + if (!baseEntries && !prEntries) return changed + if (!baseEntries || !Array.isArray(baseEntries)) { + // All PR entries are new + if (Array.isArray(prEntries)) prEntries.forEach(e => { if (e.name) changed.add(e.name) }) + return changed + } + if (!prEntries || !Array.isArray(prEntries)) { + // All base entries are deleted + baseEntries.forEach(e => { if (e.name) changed.add(e.name) }) + return changed + } + // Check for added or modified entries + for (const prEntry of prEntries) { + if (!prEntry.name) continue + const baseEntry = baseEntries.find(b => b.name === prEntry.name) + if (!baseEntry || JSON.stringify(baseEntry) !== JSON.stringify(prEntry)) { + changed.add(prEntry.name) + } + } + // Check for deleted entries + for (const baseEntry of baseEntries) { + if (!baseEntry.name) continue + if (!prEntries.find(p => p.name === baseEntry.name)) { + changed.add(baseEntry.name) + } + } + return changed +} + +/** + * Filters a NOP action's arrays to only include entries whose 'name' is in the changedNames set. + * Returns a new action with filtered arrays, or null if nothing meaningful remains. + */ +function filterActionByChangedNames (action, changedNames) { + if (!action || typeof action === 'string') return action + const { additions, deletions, modifications, ...rest } = action + + const filterArray = (arr) => { + if (!arr || !Array.isArray(arr)) return arr + return arr.filter(entry => { + if (!entry || typeof entry !== 'object') return true + // Keep entries whose name is in the changed set + if (entry.name && changedNames.has(entry.name)) return true + // Keep entries without a name field (structural entries like conditions) + if (!entry.name) return true + return false + }) + } + + const filtered = { + ...rest, + additions: filterArray(additions), + deletions: filterArray(deletions), + modifications: filterArray(modifications) + } + + // Return null if everything was filtered out + if (isEmptyChange(filtered)) return null + return filtered +} + +function buildChangeSections (changes, baseConfig, config) { + return Object.keys(changes).map(plugin => { + const repoSections = [] + Object.keys(changes[plugin]).forEach(repo => { + const targetMap = new Map() + changes[plugin][repo].forEach(action => { + targetsForAction(plugin, repo, action, baseConfig, config).forEach(target => { + if (!targetMap.has(target.target)) { + targetMap.set(target.target, { + target: target.target, + rows: [] + }) + } + targetMap.get(target.target).rows.push(...target.rows) + }) + }) + repoSections.push({ + repo, + targets: Array.from(targetMap.values()).filter(target => target.rows.length > 0) + }) + }) + + const filteredRepoSections = repoSections.filter(repoSection => repoSection.targets.length > 0) + const changeCount = filteredRepoSections.reduce((count, repoSection) => { + return count + repoSection.targets.reduce((targetCount, target) => targetCount + target.rows.length, 0) + }, 0) + const targetCount = filteredRepoSections.reduce((count, repoSection) => count + repoSection.targets.length, 0) + const repoCount = filteredRepoSections.length + const targetSingular = plugin.toLowerCase() === 'rulesets' ? 'policy' : 'setting' + const targetPlural = plugin.toLowerCase() === 'rulesets' ? 'policies' : 'settings' + const impactSummary = `${repoCount} ${pluralize(repoCount, 'repo', 'repos')}, ${targetCount} ${pluralize(targetCount, targetSingular, targetPlural)} changed` + return { + plugin, + repoSections: filteredRepoSections, + repoCount, + targetCount, + changeCount, + impactSummary, + summary: `${plugin} - ${impactSummary}` + } + }).filter(section => section.repoSections.length > 0) +} + +function renderChangeSections (changeSections) { + return changeSections.map(section => { + const repoBlocks = section.repoSections.map(repoSection => { + const targetBlocks = repoSection.targets.map(target => { + return `- ${markdownInlineCode(target.target)}\n${renderFieldChangeList(target.rows, ' ')}` + }) + return `**${markdownText(displayRepoName(repoSection.repo))}**\n${targetBlocks.join('\n')}` + }) + + return `
\n${escapeHtml(section.plugin)} — ${escapeHtml(section.impactSummary)}\n\n${repoBlocks.join('\n\n')}\n\n
` + }) +} + +function affectedRepoCount (changeSections) { + return new Set(changeSections.flatMap(section => { + return section.repoSections.map(repoSection => displayRepoName(repoSection.repo)) + })).size +} + +function displayRepoName (repo) { + return repo && repo.endsWith('(org)') ? env.ADMIN_REPO : repo +} + +function renderFieldChangeList (rows, indent = '') { + return rows.map(row => { + const marker = changeMarker(row.change) + if (row.change === 'Info') { + return `${indent}- ${marker} ${markdownText(row.after || row.before || row.field)}` + } + if (row.change === 'Modified') { + return `${indent}- ${marker} ${markdownInlineCode(row.field)}\n${indent} - before: ${markdownInlineCode(row.before, row.after)}\n${indent} - after: ${markdownInlineCode(row.after, row.before)}` + } + const value = row.change === 'Deleted' ? row.before : row.after + return `${indent}- ${marker} ${markdownInlineCode(row.field)}: ${markdownInlineCode(value)}` + }).join('\n') +} + +function changeMarker (change) { + if (change === 'Added') return '+' + if (change === 'Deleted') return '-' + if (change === 'Modified') return '~' + return 'i' +} + +function targetsForAction (plugin, repo, action, baseConfig, config) { + if (typeof action === 'string') { + return [createTarget(plugin, [createFieldChangeRow('Info', 'message', '', action)])] + } + + const configTargets = targetsFromConfigDiff(plugin, repo, action, baseConfig, config) + if (configTargets) return configTargets + + const additions = normalizeChangeEntries(action && action.additions) + const deletions = normalizeChangeEntries(action && action.deletions) + const modifications = normalizeChangeEntries(action && action.modifications) + const usedDeletions = new Set() + const targets = [] + + additions.forEach(entry => { + const target = getChangeTarget(entry, plugin) + targets.push(createTarget(target, rowsForAddedOrDeleted('Added', entry, target))) + }) + + modifications.forEach((entry, index) => { + const target = getChangeTarget(entry, plugin) + const match = findMatchingDeletion(entry, index, modifications, deletions, usedDeletions) + if (match.index !== -1) usedDeletions.add(match.index) + targets.push(createTarget(target, rowsForModification(match.entry, entry, target))) + }) + + deletions.forEach((entry, index) => { + if (usedDeletions.has(index)) return + const target = getChangeTarget(entry, plugin) + targets.push(createTarget(target, rowsForAddedOrDeleted('Deleted', entry, target))) + }) + + if (targets.length === 0 && action && action.msg) { + return [createTarget(plugin, [createFieldChangeRow('Info', 'message', '', action.msg)])] + } + + return targets +} + +function targetsFromConfigDiff (plugin, repo, action, baseConfig, config) { + if (!baseConfig || !config || !action || typeof action === 'string') return null + + const pluginSection = plugin.toLowerCase() + const isOrgRulesets = repo && repo.endsWith('(org)') && pluginSection === 'rulesets' + const baseEntries = baseConfig[pluginSection] + const prEntries = config[pluginSection] + + if (!isOrgRulesets) return null + if (!Array.isArray(baseEntries) || !Array.isArray(prEntries)) return null + + const actionNames = getActionEntryNames(action) + if (actionNames.size === 0) return null + + const changedNames = new Set(Array.from(getChangedEntryNames(baseEntries, prEntries)).filter(name => actionNames.has(name))) + if (changedNames.size === 0) return null + + const targets = [] + Array.from(changedNames).sort().forEach(name => { + const oldEntry = findEntryByIdentity(baseEntries, name) + const newEntry = findEntryByIdentity(prEntries, name) + let rows = [] + + if (oldEntry && newEntry) { + rows = rowsForModification(oldEntry, newEntry, name) + } else if (newEntry) { + rows = rowsForAddedOrDeleted('Added', newEntry, name) + } else if (oldEntry) { + rows = rowsForAddedOrDeleted('Deleted', oldEntry, name) + } + + if (rows.length > 0) targets.push(createTarget(name, rows)) + }) + + return targets.length > 0 ? targets : null +} + +function getActionEntryNames (action) { + const names = new Set() + ;['additions', 'deletions', 'modifications'].forEach(actionField => { + normalizeChangeEntries(action[actionField]).forEach(entry => { + const identity = getEntryIdentityValue(entry) + if (identity) names.add(identity) + }) + }) + return names +} + +function findEntryByIdentity (entries, identity) { + return entries.find(entry => getEntryIdentityValue(entry) === identity) +} + +function createTarget (target, rows) { + return { + target, + rows: rows.filter(row => row) + } +} + +function normalizeChangeEntries (value) { + if (isDeepEmpty(value)) return [] + return Array.isArray(value) ? value.filter(entry => !isDeepEmpty(entry)) : [value] +} + +function findMatchingDeletion (entry, index, modifications, deletions, usedDeletions) { + const identity = getChangeIdentity(entry) + if (identity) { + const matchIndex = deletions.findIndex((deletion, deletionIndex) => { + if (usedDeletions.has(deletionIndex)) return false + return getChangeIdentity(deletion) === identity + }) + if (matchIndex !== -1) return { entry: deletions[matchIndex], index: matchIndex } + } + + if (modifications.length === 1 && deletions.length === 1 && !usedDeletions.has(0)) { + return { entry: deletions[0], index: 0 } + } + + return { entry: null, index: -1 } +} + +function getChangeIdentity (entry) { + if (!entry || typeof entry !== 'object' || Array.isArray(entry)) return null + const field = MergeDeep.NAME_FIELDS.find(field => Object.prototype.hasOwnProperty.call(entry, field)) + if (!field) return null + return `${field}:${formatValue(entry[field]).text}` +} + +function getChangeTarget (entry, fallback) { + if (!entry || typeof entry !== 'object' || Array.isArray(entry)) return formatValue(entry).text || fallback + return getEntryIdentityValue(entry) || fallback +} + +function getEntryIdentityValue (entry) { + if (!entry || typeof entry !== 'object' || Array.isArray(entry)) return null + const field = MergeDeep.NAME_FIELDS.find(field => Object.prototype.hasOwnProperty.call(entry, field)) + return field ? formatValue(entry[field]).text : null +} + +function rowsForAddedOrDeleted (change, entry, target) { + const flattened = flattenForSummary(entry, true) + const fields = Object.keys(flattened) + if (fields.length === 0) return [createFieldChangeRow(change, 'value', change === 'Added' ? '' : target, change === 'Added' ? target : '')] + + return fields.map(path => { + const value = flattened[path] + return createFieldChangeRow(change, path, change === 'Deleted' ? value : '', change === 'Deleted' ? '' : value) + }) +} + +function rowsForModification (oldEntry, newEntry, target) { + if (!oldEntry || typeof oldEntry !== 'object' || !newEntry || typeof newEntry !== 'object') { + return rowsForAddedOrDeleted('Modified', newEntry, target) + } + + const oldPaths = flattenForSummary(oldEntry, true) + const newPaths = flattenForSummary(newEntry, true) + const paths = Array.from(new Set([...Object.keys(oldPaths), ...Object.keys(newPaths)])).sort() + const rows = paths.map(path => { + const hasOld = Object.prototype.hasOwnProperty.call(oldPaths, path) + const hasNew = Object.prototype.hasOwnProperty.call(newPaths, path) + if (hasOld && hasNew && comparableValue(oldPaths[path]) !== comparableValue(newPaths[path])) { + return createFieldChangeRow('Modified', path, oldPaths[path], newPaths[path]) + } + if (!hasOld && hasNew) { + return createFieldChangeRow('Added', path, '', newPaths[path]) + } + if (hasOld && !hasNew) { + return createFieldChangeRow('Deleted', path, oldPaths[path], '') + } + return null + }).filter(row => row) + + if (rows.length > 0) return rows + return rowsForAddedOrDeleted('Modified', newEntry, target) +} + +function createFieldChangeRow (change, field, before, after) { + return { + change, + field, + before, + after + } +} + +function flattenForSummary (value, skipRootIdentity = false, prefix = '') { + if (value === null || value === undefined || typeof value !== 'object') { + return { [prefix || 'value']: formatValue(value) } + } + + if (Array.isArray(value)) { + return { [prefix || 'value']: formatValue(value) } + } + + const result = {} + Object.keys(value).forEach(key => { + if (!prefix && skipRootIdentity && MergeDeep.NAME_FIELDS.includes(key)) return + const path = prefix ? `${prefix}.${key}` : key + const child = value[key] + + if (child && typeof child === 'object' && !Array.isArray(child)) { + Object.assign(result, flattenForSummary(child, false, path)) + } else { + result[path] = formatValue(child) + } + }) + + return result +} + +function formatValue (value) { + if (value && typeof value === 'object' && Object.prototype.hasOwnProperty.call(value, 'text')) return value + if (value === null) return { text: 'null', compare: 'null' } + if (value === undefined) return { text: '', compare: '' } + if (typeof value === 'string') return { text: value, compare: value } + if (typeof value === 'number' || typeof value === 'boolean') return { text: `${value}`, compare: `${value}` } + if (Array.isArray(value) && value.every(item => item === null || ['string', 'number', 'boolean'].includes(typeof item))) { + const text = value.map(item => formatValue(item).text).join(', ') + return { text, compare: text } + } + const json = JSON.stringify(value) + return { + text: truncate(json, 180), + compare: json + } +} + +function comparableValue (value) { + const displayValue = formatValue(value) + return Object.prototype.hasOwnProperty.call(displayValue, 'compare') ? displayValue.compare : displayValue.text +} + +function truncate (value, limit = 180) { + if (!value || value.length <= limit) return value + return `${value.substring(0, limit - 3)}...` +} + +function truncateAroundDifference (value, otherValue, limit = 180) { + if (!value || value.length <= limit) return value + if (!otherValue || value === otherValue) return truncate(value, limit) + + let prefixLength = 0 + while ( + prefixLength < value.length && + prefixLength < otherValue.length && + value[prefixLength] === otherValue[prefixLength] + ) { + prefixLength++ + } + + let suffixLength = 0 + while ( + suffixLength < value.length - prefixLength && + suffixLength < otherValue.length - prefixLength && + value[value.length - 1 - suffixLength] === otherValue[otherValue.length - 1 - suffixLength] + ) { + suffixLength++ + } + + const contextLength = Math.floor((limit - 6) / 2) + const start = Math.max(0, prefixLength - contextLength) + const end = Math.min(value.length, value.length - suffixLength + contextLength) + const prefix = start > 0 ? '...' : '' + const suffix = end < value.length ? '...' : '' + return truncate(`${prefix}${value.substring(start, end)}${suffix}`, limit) +} + +function truncateWithSuffix (value, limit, suffix) { + if (!value || value.length <= limit) return value + return `${value.substring(0, limit - suffix.length)}${suffix}` +} + +function pluralize (count, singular, plural) { + return count === 1 ? singular : plural +} + +function markdownInlineCode (value, comparedWith) { + return `\`${markdownText(value, comparedWith).replaceAll('`', '\\`')}\`` +} + +function markdownText (value, comparedWith) { + const displayValue = formatValue(value) + const otherDisplayValue = comparedWith === undefined ? null : formatValue(comparedWith) + const text = otherDisplayValue + ? truncateAroundDifference(displayValue.compare || displayValue.text, otherDisplayValue.compare || otherDisplayValue.text) + : displayValue.text + return escapeHtml(text) + .replaceAll('\n', ' ') +} + +function escapeHtml (value) { + return `${value}` + .replaceAll('&', '&') + .replaceAll('<', '<') + .replaceAll('>', '>') +} + class Settings { static fileCache = {} - static async syncAll (nop, context, repo, config, ref) { - const settings = new Settings(nop, context, repo, config, ref) + static async syncAll (nop, context, repo, config, ref, baseConfig, changedFiles = {}) { + const settings = new Settings(nop, context, repo, config, ref, null, baseConfig) + settings.setChangedConfigTargets(changedFiles.repos, changedFiles.subOrgs) try { await settings.loadConfigs() + settings.trackChangedReposFromSubOrgConfigs() // settings.repoConfigs = await settings.getRepoConfigs() await settings.updateOrg() await settings.updateAll() @@ -42,10 +513,14 @@ class Settings { } } - static async syncSelectedRepos (nop, context, repos, subOrgs, config, ref) { - const settings = new Settings(nop, context, context.repo(), config, ref) + static async syncSelectedRepos (nop, context, repos, subOrgs, config, ref, baseConfig) { + const settings = new Settings(nop, context, context.repo(), config, ref, null, baseConfig) + settings.setChangedConfigTargets(repos, subOrgs) try { + settings.subOrgConfigs = await settings.getSubOrgConfigs() + settings.trackChangedReposFromSubOrgConfigs() + // Apply org-level settings (e.g., rulesets) first, matching syncAll behavior await settings.updateOrg() @@ -91,13 +566,14 @@ class Settings { await settings.handleResults() } - constructor (nop, context, repo, config, ref, suborg) { + constructor (nop, context, repo, config, ref, suborg, baseConfig) { this.ref = ref this.context = context this.installation_id = context.payload.installation.id this.github = context.octokit this.repo = repo this.config = config + this.baseConfig = baseConfig || null this.nop = nop this.suborgChange = !!suborg // If suborg config has been updated, do not load the entire suborg config, and only process repos restricted to it. @@ -129,6 +605,41 @@ class Settings { this.mergeDeep = new MergeDeep(this.log, this.github, [], this.configvalidators, this.overridevalidators) } + setChangedConfigTargets (changedRepos = [], changedSubOrgs = []) { + const repoNames = Array.isArray(changedRepos) + ? changedRepos.map(repo => repo && repo.repo).filter(Boolean) + : [] + + this.changedRepoNames = new Set(repoNames) + this.changedSubOrgConfigs = Array.isArray(changedSubOrgs) ? changedSubOrgs : [] + } + + trackChangedReposFromSubOrgConfigs () { + if (!Array.isArray(this.changedSubOrgConfigs) || this.changedSubOrgConfigs.length === 0 || !this.subOrgConfigs) { + return + } + + const changedSubOrgPaths = new Set( + this.changedSubOrgConfigs + .map(subOrg => subOrg && subOrg.path) + .filter(Boolean) + ) + + if (changedSubOrgPaths.size === 0) { + return + } + + if (!this.changedRepoNames) { + this.changedRepoNames = new Set() + } + + Object.entries(this.subOrgConfigs).forEach(([repoName, subOrgConfig]) => { + if (subOrgConfig && subOrgConfig.source && changedSubOrgPaths.has(subOrgConfig.source)) { + this.changedRepoNames.add(repoName) + } + }) + } + // Create a check in the Admin repo for safe-settings. async createCheckRun () { const startTime = new Date() @@ -203,6 +714,52 @@ class Settings { }) }) + // Filter results to only show PR-introduced changes (not pre-existing drift) + 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 + + const isOrgLevel = res.repo && res.repo.endsWith('(org)') + const pluginSection = res.plugin ? res.plugin.toLowerCase() : null + + if (isOrgLevel && pluginSection === 'rulesets') { + // For org-level rulesets: only include rulesets whose config definition changed + const changedNames = getChangedEntryNames(this.baseConfig.rulesets, this.config.rulesets) + if (changedNames.size === 0) return false + const filtered = filterActionByChangedNames(res.action, changedNames) + if (!filtered) return false + res.action = filtered + return true + } + + if (!isOrgLevel && pluginSection) { + // Repos whose override files changed are always relevant + if (this.changedRepoNames && this.changedRepoNames.has(res.repo)) { + return true + } + + // Repo-level rulesets come from override files (not global config.rulesets) + // so don't compare against org-level rulesets — just filter them as drift + // when no override files changed for this repo + if (pluginSection === 'rulesets') { + return false + } + + // For other repo-level plugins: check if the global config section changed + const baseSection = this.baseConfig[pluginSection] + const prSection = this.config[pluginSection] + if (baseSection !== undefined && prSection !== undefined) { + if (JSON.stringify(baseSection) === JSON.stringify(prSection)) { + return false + } + } + } + + return true + }) + } + let error = false // Different logic const stats = { @@ -232,68 +789,87 @@ class Settings { stats.errors[res.repo] = [] } stats.errors[res.repo].push(res.action) - } else if (!(res.action?.additions === null && res.action?.deletions === null && res.action?.modifications === null)) { + } else if (!isEmptyChange(res.action)) { if (!stats.changes[res.plugin]) { stats.changes[res.plugin] = {} } if (!stats.changes[res.plugin][res.repo]) { stats.changes[res.plugin][res.repo] = [] } - stats.changes[res.plugin][res.repo].push(`${res.action}`) + stats.changes[res.plugin][res.repo].push(res.action) } } }) this.log.debug(`Stats ${JSON.stringify(this.results, null, 2)}`) - const table = ` - - - - - - - - - - - - ` + stats.changeSections = buildChangeSections(stats.changes, this.baseConfig, this.config) + stats.reposAffected = affectedRepoCount(stats.changeSections) + stats.changeDetails = stats.changeSections.length > 0 ? renderChangeSections(stats.changeSections).join('\n\n') : '' + stats.checkRunDetails = stats.changeDetails.length > 50000 + ? 'Detailed changed-field output is available in the pull request comment.' + : stats.changeDetails const renderedCommentMessage = await eta.renderString(commetMessageTemplate, stats) if (env.CREATE_PR_COMMENT === 'true') { - const summary = ` -#### :robot: Safe-Settings config changes detected: - -${this.results.reduce((x, y) => { - if (!y) { - return x - } - if (y.type === 'ERROR') { - error = true - return `${x} -` - } else if (y.action.additions === null && y.action.deletions === null && y.action.modifications === null) { - return `${x}` + const pluginSectionList = renderChangeSections(stats.changeSections) + + const errorRepos = Object.keys(stats.errors) + const errorSection = errorRepos.length === 0 + ? '### Errors\n`None`' + : `### Errors\n
\n⚠️ Errors — ${errorRepos.length} repo(s) affected\n\n${ + errorRepos.map(repo => + `**${repo}**:\n${stats.errors[repo].map(e => `* ${e.msg}`).join('\n')}` + ).join('\n\n') + }\n\n
` + + const allSections = stats.changeSections.length === 0 + ? ['_No changes to apply._', errorSection] + : [...pluginSectionList, errorSection] + + const repoCount = Object.keys(stats.reposProcessed).length + const makeHeader = (page, total) => + total > 1 + ? `#### :robot: Safe-Settings config changes detected (${page}/${total}):\n\n**Repos considered:** ${repoCount}\n**Repos affected:** ${stats.reposAffected}\n\n` + : `#### :robot: Safe-Settings config changes detected:\n\n**Repos considered:** ${repoCount}\n**Repos affected:** ${stats.reposAffected}\n\n` + + const HEADER_OVERHEAD = 80 + const BODY_LIMIT = COMMENT_LIMIT - HEADER_OVERHEAD + + const pages = [] + let currentChunks = [] + let currentLength = 0 + + for (const section of allSections) { + const sectionLength = section.length + 2 + if (currentChunks.length > 0 && currentLength + sectionLength > BODY_LIMIT) { + pages.push(currentChunks.join('\n\n')) + currentChunks = [section] + currentLength = sectionLength } else { - if (y.action === undefined) { - return `${x}` - } - return `${x} -` + currentChunks.push(section) + currentLength += sectionLength } - }, table)} -` + } + if (currentChunks.length > 0) { + pages.push(currentChunks.join('\n\n')) + } + const totalPages = pages.length const pullRequest = payload.check_run.check_suite.pull_requests[0] - await this.github.rest.issues.createComment({ - owner: payload.repository.owner.login, - repo: payload.repository.name, - issue_number: pullRequest.number, - body: summary.length > 55536 ? `${summary.substring(0, 55536)}... (too many changes to report)` : summary - }) + for (let i = 0; i < pages.length; i++) { + const header = makeHeader(i + 1, totalPages) + const body = header + pages[i] + const safeBody = truncateWithSuffix(body, COMMENT_LIMIT, '... (too many changes to report)') + await this.github.rest.issues.createComment({ + owner: payload.repository.owner.login, + repo: payload.repository.name, + issue_number: pullRequest.number, + body: safeBody + }) + } } const params = { @@ -305,7 +881,7 @@ ${this.results.reduce((x, y) => { completed_at: new Date().toISOString(), output: { title: error ? 'Safe-Settings Dry-Run Finished with Error' : 'Safe-Settings Dry-Run Finished with success', - summary: renderedCommentMessage.length > 55536 ? `${renderedCommentMessage.substring(0, 55536)}... (too many changes to report)` : renderedCommentMessage + summary: truncateWithSuffix(renderedCommentMessage, COMMENT_LIMIT, '... (too many changes to report)') } } @@ -323,6 +899,9 @@ ${this.results.reduce((x, y) => { if (rulesetsConfig) { const RulesetsPlugin = Settings.PLUGINS.rulesets return new RulesetsPlugin(this.nop, this.github, this.repo, rulesetsConfig, this.log, this.errors, SCOPE.ORG).sync().then(res => { + if (this.nop && Array.isArray(res)) { + res.forEach(r => { if (r) r.repo = `${this.repo.owner} (org)` }) + } this.appendToResults(res) }) } @@ -984,13 +1563,6 @@ ${this.results.reduce((x, y) => { } } -function prettify (obj) { - if (obj === null || obj === undefined) { - return '' - } - return JSON.stringify(obj, null, 2).replaceAll('\n', '
').replaceAll(' ', ' ') -} - Settings.FILE_NAME = path.posix.join(CONFIG_PATH, env.SETTINGS_FILE_PATH) Settings.FILE_PATH = path.posix.join(CONFIG_PATH, env.SETTINGS_FILE_PATH) Settings.SUB_ORG_PATTERN = new Glob(`${CONFIG_PATH}/suborgs/*.yml`) @@ -1012,3 +1584,7 @@ Settings.PLUGINS = { } module.exports = Settings +module.exports.isEmptyChange = isEmptyChange +module.exports.isDeepEmpty = isDeepEmpty +module.exports.getChangedEntryNames = getChangedEntryNames +module.exports.filterActionByChangedNames = filterActionByChangedNames diff --git a/schema/dereferenced/repos.json b/schema/dereferenced/repos.json index 9213456a..7eacb4bd 100644 --- a/schema/dereferenced/repos.json +++ b/schema/dereferenced/repos.json @@ -1563,7 +1563,7 @@ "actor_id": { "type": "integer", "nullable": true, - "description": "The ID of the actor that can bypass a ruleset. Required for `Integration`, `RepositoryRole`, and `Team` actor types. If `actor_type` is `OrganizationAdmin`, `actor_id` is ignored. If `actor_type` is `DeployKey`, this should be null. `OrganizationAdmin` is not applicable for personal repositories." + "description": "The ID of the actor that can bypass a ruleset. Required for `Integration`, `RepositoryRole`, `Team`, and `User` actor types. If `actor_type` is `OrganizationAdmin`, `actor_id` is ignored. If `actor_type` is `DeployKey`, this should be null. `OrganizationAdmin` is not applicable for personal repositories." }, "actor_type": { "type": "string", @@ -1572,7 +1572,8 @@ "OrganizationAdmin", "RepositoryRole", "Team", - "DeployKey" + "DeployKey", + "User" ], "description": "The type of actor that can bypass a ruleset." }, diff --git a/schema/dereferenced/settings.json b/schema/dereferenced/settings.json index 8698b201..5279f02c 100644 --- a/schema/dereferenced/settings.json +++ b/schema/dereferenced/settings.json @@ -308,7 +308,7 @@ } }, "force_create": { - "description": "Force create the repository even if it already exists", + "description": "Force create the repository", "type": "boolean" }, "template": { @@ -440,15 +440,6 @@ "notifications_disabled" ] }, - "permission": { - "type": "string", - "description": "**Closing down notice**. The permission that new repositories will be added to the team with when none is specified.", - "enum": [ - "pull", - "push" - ], - "default": "pull" - }, "parent_team_id": { "type": "integer", "description": "The ID of a team to set as the parent team." @@ -774,7 +765,7 @@ "actor_id": { "type": "integer", "nullable": true, - "description": "The ID of the actor that can bypass a ruleset. Required for `Integration`, `RepositoryRole`, and `Team` actor types. If `actor_type` is `OrganizationAdmin`, `actor_id` is ignored. If `actor_type` is `DeployKey`, this should be null. `OrganizationAdmin` is not applicable for personal repositories." + "description": "The ID of the actor that can bypass a ruleset. Required for `Integration`, `RepositoryRole`, `Team`, and `User` actor types. If `actor_type` is `OrganizationAdmin`, `actor_id` is ignored. If `actor_type` is `DeployKey`, this should be null. `OrganizationAdmin` is not applicable for personal repositories." }, "actor_type": { "type": "string", @@ -783,7 +774,8 @@ "OrganizationAdmin", "RepositoryRole", "Team", - "DeployKey" + "DeployKey", + "User" ], "description": "The type of actor that can bypass a ruleset." }, @@ -2286,7 +2278,7 @@ } }, "force_create": { - "description": "Force create the repository even if it already exists", + "description": "Force create the repository", "type": "boolean" }, "template": { @@ -2720,7 +2712,7 @@ "actor_id": { "type": "integer", "nullable": true, - "description": "The ID of the actor that can bypass a ruleset. Required for `Integration`, `RepositoryRole`, and `Team` actor types. If `actor_type` is `OrganizationAdmin`, `actor_id` is ignored. If `actor_type` is `DeployKey`, this should be null. `OrganizationAdmin` is not applicable for personal repositories." + "description": "The ID of the actor that can bypass a ruleset. Required for `Integration`, `RepositoryRole`, `Team`, and `User` actor types. If `actor_type` is `OrganizationAdmin`, `actor_id` is ignored. If `actor_type` is `DeployKey`, this should be null. `OrganizationAdmin` is not applicable for personal repositories." }, "actor_type": { "type": "string", @@ -2729,7 +2721,8 @@ "OrganizationAdmin", "RepositoryRole", "Team", - "DeployKey" + "DeployKey", + "User" ], "description": "The type of actor that can bypass a ruleset." }, diff --git a/schema/dereferenced/suborgs.json b/schema/dereferenced/suborgs.json index 0267bf7a..601b32be 100644 --- a/schema/dereferenced/suborgs.json +++ b/schema/dereferenced/suborgs.json @@ -1597,7 +1597,7 @@ "actor_id": { "type": "integer", "nullable": true, - "description": "The ID of the actor that can bypass a ruleset. Required for `Integration`, `RepositoryRole`, and `Team` actor types. If `actor_type` is `OrganizationAdmin`, `actor_id` is ignored. If `actor_type` is `DeployKey`, this should be null. `OrganizationAdmin` is not applicable for personal repositories." + "description": "The ID of the actor that can bypass a ruleset. Required for `Integration`, `RepositoryRole`, `Team`, and `User` actor types. If `actor_type` is `OrganizationAdmin`, `actor_id` is ignored. If `actor_type` is `DeployKey`, this should be null. `OrganizationAdmin` is not applicable for personal repositories." }, "actor_type": { "type": "string", @@ -1606,7 +1606,8 @@ "OrganizationAdmin", "RepositoryRole", "Team", - "DeployKey" + "DeployKey", + "User" ], "description": "The type of actor that can bypass a ruleset." }, diff --git a/test/unit/lib/handleResults.test.js b/test/unit/lib/handleResults.test.js new file mode 100644 index 00000000..b138ec1c --- /dev/null +++ b/test/unit/lib/handleResults.test.js @@ -0,0 +1,1165 @@ +/* eslint-disable no-undef */ +'use strict' + +const Settings = require('../../../lib/settings') +const { isEmptyChange, isDeepEmpty } = require('../../../lib/settings') +const env = require('../../../lib/env') + +// --------------------------------------------------------------------------- +// Shared test helpers +// --------------------------------------------------------------------------- + +function makeNopResult ({ repo = 'my-repo', plugin = 'labels', additions = ['label-a'], deletions = null, modifications = null } = {}) { + return { + type: 'NOP', + plugin, + repo, + endpoint: 'https://api.github.com/repos/test-org/my-repo/labels', + body: {}, + action: { + additions, + deletions, + modifications + } + } +} + +function makeErrorResult ({ repo = 'my-repo', plugin = 'labels', msg = 'Something went wrong' } = {}) { + return { + type: 'ERROR', + plugin, + repo, + endpoint: 'https://api.github.com/repos/test-org/my-repo/labels', + body: {}, + action: { + additions: null, + deletions: null, + modifications: null, + msg + } + } +} + +function buildContext (overrides = {}) { + const createComment = jest.fn().mockResolvedValue({}) + const checksUpdate = jest.fn().mockResolvedValue({}) + + const context = { + payload: { + installation: { id: 1 }, + check_run: { + id: 42, + check_suite: { + pull_requests: [{ number: 7 }] + } + }, + repository: { + owner: { login: 'test-org' }, + name: 'admin' + } + }, + octokit: { + rest: { + issues: { createComment }, + checks: { update: checksUpdate } + } + }, + log: { + debug: jest.fn(), + info: jest.fn(), + error: jest.fn() + }, + ...overrides + } + + return { context, createComment, checksUpdate } +} + +function buildSettings (context, results = [], config = {}, baseConfig = null) { + const settings = new Settings( + /* nop */ true, + context, + { owner: 'test-org', repo: 'admin' }, + /* config */ config, + /* ref */ 'main', + /* suborg */ null, + /* baseConfig */ baseConfig + ) + settings.results = results + return settings +} + +function getCommentBodies (createComment) { + expect(createComment).toHaveBeenCalled() + return createComment.mock.calls.map(([request]) => request.body) +} + +function getCombinedCommentBody (createComment) { + return getCommentBodies(createComment).join('\n\n') +} + +// --------------------------------------------------------------------------- +// Restore env after each test +// --------------------------------------------------------------------------- + +let originalCreatePrComment + +beforeEach(() => { + originalCreatePrComment = env.CREATE_PR_COMMENT + env.CREATE_PR_COMMENT = 'true' +}) + +afterEach(() => { + env.CREATE_PR_COMMENT = originalCreatePrComment +}) + +// --------------------------------------------------------------------------- +// Test Plan +// --------------------------------------------------------------------------- + +/* + * Code Summary + * ------------ + * handleResults() is the final step of every sync flow. When nop=true it + * renders a human-readable markdown summary of all NopCommand results, + * optionally posts that summary as a PR comment via the GitHub Issues API, and + * always updates a check run via the Checks API with the Eta-rendered template. + * + * Primary concern: the two API calls receive correctly shaped bodies, edge + * cases (empty results, all-null actions, errors) are handled, and the + * CREATE_PR_COMMENT env flag is respected. + */ + +describe('handleResults()', () => { + // ------------------------------------------------------------------------- + // Test 1 — empty state + // ------------------------------------------------------------------------- + describe('renders empty state without changes', () => { + it('PR comment body contains _No changes to apply._', async () => { + // Arrange + const { context, createComment } = buildContext() + const settings = buildSettings(context, []) + + // Act + await settings.handleResults() + + // Assert + const body = getCombinedCommentBody(createComment) + expect(body).toContain('_No changes to apply._') + }) + + it('check run summary contains "No changes to apply"', async () => { + // Arrange + const { context, checksUpdate } = buildContext() + const settings = buildSettings(context, []) + + // Act + await settings.handleResults() + + // Assert + expect(checksUpdate).toHaveBeenCalledTimes(1) + const summary = checksUpdate.mock.calls[0][0].output.summary + expect(summary).toMatch(/No changes to apply/i) + }) + }) + + // ------------------------------------------------------------------------- + // Test 2 — PR comment contains
/ tags with a real result + // ------------------------------------------------------------------------- + describe('PR comment body contains details tags', () => { + it('wraps per-plugin changes in a collapsible
section', async () => { + // Arrange + const { context, createComment } = buildContext() + const result = makeNopResult({ repo: 'my-repo', plugin: 'labels', additions: ['bug', 'enhancement'] }) + const settings = buildSettings(context, [result]) + + // Act + await settings.handleResults() + + // Assert + const body = getCombinedCommentBody(createComment) + expect(body).toContain('
') + expect(body).toContain('') + }) + }) + + // ------------------------------------------------------------------------- + // Test 3 — check run summary contains
/ tags + // ------------------------------------------------------------------------- + describe('check run summary contains details tags', () => { + it('wraps changes section in collapsible HTML when there are results', async () => { + // Arrange + const { context, checksUpdate } = buildContext() + const result = makeNopResult({ repo: 'my-repo', plugin: 'labels', additions: ['bug'] }) + const settings = buildSettings(context, [result]) + + // Act + await settings.handleResults() + + // Assert + const summary = checksUpdate.mock.calls[0][0].output.summary + expect(summary).toContain('
') + expect(summary).toContain('') + }) + }) + + // ------------------------------------------------------------------------- + // Test 4 — output stays under 55536 chars with 200 results + // ------------------------------------------------------------------------- + describe('output stays under 55536 chars with many repos', () => { + it('each comment page and the summary are under the limit and are not truncated', async () => { + // Arrange + const { context, createComment, checksUpdate } = buildContext() + + const REPOS = 40 + const PLUGINS = ['labels', 'teams', 'collaborators', 'branches', 'environments'] + + const results = [] + for (let r = 0; r < REPOS; r++) { + for (const plugin of PLUGINS) { + results.push(makeNopResult({ repo: `repo-${r}`, plugin, additions: ['item-a', 'item-b'] })) + } + } + + const settings = buildSettings(context, results) + + // Act + await settings.handleResults() + + // Assert — paginated bodies + const bodies = getCommentBodies(createComment) + expect(bodies.length).toBeGreaterThan(0) + bodies.forEach(body => { + expect(body.length).toBeLessThanOrEqual(55536) + expect(body).not.toContain('too many changes to report') + }) + + // Assert — summary + const summary = checksUpdate.mock.calls[0][0].output.summary + expect(summary.length).toBeLessThan(55536) + expect(summary).not.toContain('too many changes to report') + }) + + it('truncates oversized PR comments without exceeding the limit', async () => { + const { context, createComment } = buildContext() + const result = makeNopResult({ + repo: 'huge-repo', + plugin: 'labels', + additions: [{ name: 'huge-label', description: 'x'.repeat(60000) }] + }) + const settings = buildSettings(context, [result]) + + await settings.handleResults() + + const bodies = getCommentBodies(createComment) + bodies.forEach(body => { + expect(body.length).toBeLessThanOrEqual(55536) + }) + expect(bodies.some(body => body.includes('too many changes to report'))).toBe(true) + }) + + it('truncates oversized check-run summaries without exceeding the limit', async () => { + const { context, checksUpdate } = buildContext() + const errorResult = makeErrorResult({ + repo: 'broken-repo', + msg: 'x'.repeat(60000) + }) + const settings = buildSettings(context, [errorResult]) + + await settings.handleResults() + + const summary = checksUpdate.mock.calls[0][0].output.summary + expect(summary.length).toBeLessThanOrEqual(55536) + expect(summary).toContain('too many changes to report') + }) + }) + + // ------------------------------------------------------------------------- + // Test 5 — errors are wrapped in a collapsible section + // ------------------------------------------------------------------------- + describe('errors are wrapped in collapsible section', () => { + it('body contains ⚠️ Errors heading and a
element when there is an ERROR result', async () => { + // Arrange + const { context, createComment } = buildContext() + const result = makeErrorResult({ repo: 'broken-repo', msg: 'API rate limit exceeded' }) + const settings = buildSettings(context, [result]) + + // Act + await settings.handleResults() + + // Assert + const body = getCombinedCommentBody(createComment) + expect(body).toContain('⚠️ Errors') + expect(body).toContain('
') + }) + }) + + // ------------------------------------------------------------------------- + // Test 6 — repos with all-null actions are omitted from output + // ------------------------------------------------------------------------- + describe('repos with all-null actions are omitted from output', () => { + it('a result with additions/deletions/modifications all null does not appear in the PR comment body', async () => { + // Arrange + const { context, createComment } = buildContext() + const nullResult = makeNopResult({ + repo: 'silent-repo', + plugin: 'labels', + additions: null, + deletions: null, + modifications: null + }) + const settings = buildSettings(context, [nullResult]) + + // Act + await settings.handleResults() + + // Assert + const body = getCombinedCommentBody(createComment) + // The repo name should not appear in the changes table + expect(body).not.toMatch(/silent-repo.*Add:/) + // There must be no changes section referencing this repo + expect(body).toContain('_No changes to apply._') + }) + + it('a result with empty-object/array action fields is filtered out', async () => { + // Arrange + const { context, createComment } = buildContext() + const emptyResult = makeNopResult({ + repo: 'empty-repo', + plugin: 'labels', + additions: {}, + deletions: [], + modifications: null + }) + const settings = buildSettings(context, [emptyResult]) + + // Act + await settings.handleResults() + + // Assert + const body = getCombinedCommentBody(createComment) + expect(body).not.toContain('empty-repo') + expect(body).toContain('_No changes to apply._') + }) + }) + + // ------------------------------------------------------------------------- + // Test 7 — org-level result rows display the admin repo + // ------------------------------------------------------------------------- + describe('org-level labeling', () => { + it('shows the admin repo name instead of the org target in output', async () => { + // Arrange + const { context, createComment } = buildContext() + const orgResult = makeNopResult({ + repo: 'test-org (org)', + plugin: 'rulesets', + additions: [{ name: 'require-pull-request-reviews' }] + }) + const settings = buildSettings(context, [orgResult]) + + // Act + await settings.handleResults() + + // Assert + const body = getCombinedCommentBody(createComment) + expect(body).toContain('admin') + expect(body).not.toContain('test-org (org)') + }) + }) + + // ------------------------------------------------------------------------- + // Test 8 — table-free rendering with trimmed changed fields + // ------------------------------------------------------------------------- + describe('table-free rendering with trimmed changed fields', () => { + it('shows affected admin target, changed policy, and field-level ruleset diff', async () => { + const { context, createComment } = buildContext() + + const baseConfig = { + rulesets: [ + { + name: 'Agent Studio - Required Workflows', + conditions: { repository_name: { include: ['agent-*'] } } + } + ] + } + const prConfig = { + rulesets: [ + { + name: 'Agent Studio - Required Workflows', + conditions: { repository_name: { include: ['mythapi-*'] } } + } + ] + } + const orgResult = { + type: 'NOP', + plugin: 'Rulesets', + repo: 'test-org (org)', + endpoint: '', + body: {}, + action: { + additions: [], + deletions: [ + { + name: 'Agent Studio - Required Workflows', + conditions: { repository_name: { include: ['agent-*'] } } + } + ], + modifications: [ + { + name: 'Agent Studio - Required Workflows', + conditions: { repository_name: { include: ['mythapi-*'] } }, + bypass_actors: [{ actor_id: 1 }] + } + ] + } + } + + const settings = buildSettings(context, [orgResult], prConfig, baseConfig) + await settings.handleResults() + + const body = getCombinedCommentBody(createComment) + expect(body).toContain('**Repos affected:** 1') + expect(body).toContain('Rulesets — 1 repo, 1 policy changed') + expect(body).toContain('**admin**') + expect(body).not.toContain('| Repo |') + expect(body).not.toContain('
MsgPluginRepoAdditionsDeletionsModifications
❗ ${y.action.msg} ${y.plugin} ${prettify(y.repo)} ${prettify(y.action.additions)} ${prettify(y.action.deletions)} ${prettify(y.action.modifications)}
✋ ${y.plugin} ${prettify(y.repo)} ${prettify(y.action.additions)} ${prettify(y.action.deletions)} ${prettify(y.action.modifications)}
') + expect(body).not.toContain('test-org (org)') + expect(body).toContain('- `Agent Studio - Required Workflows`') + expect(body).toContain(' - ~ `conditions.repository_name.include`') + expect(body).toContain(' - before: `agent-*`') + expect(body).toContain(' - after: `mythapi-*`') + expect(body).not.toContain('bypass_actors') + }) + + it('does not render config-changed rulesets absent from NOP actions', async () => { + const { context, createComment } = buildContext() + const baseConfig = { + rulesets: [ + { name: 'Rule B', conditions: { repository_name: { include: ['agent-*'] } } }, + { name: 'Rule D', enforcement: 'evaluate' } + ] + } + const prConfig = { + rulesets: [ + { name: 'Rule B', conditions: { repository_name: { include: ['mythapi-*'] } } }, + { name: 'Rule D', enforcement: 'active' } + ] + } + const orgResult = { + type: 'NOP', + plugin: 'Rulesets', + repo: 'test-org (org)', + endpoint: '', + body: {}, + action: { + additions: [], + deletions: [{ name: 'Rule B', conditions: { repository_name: { include: ['agent-*'] } } }], + modifications: [{ name: 'Rule B', conditions: { repository_name: { include: ['mythapi-*'] } } }] + } + } + const settings = buildSettings(context, [orgResult], prConfig, baseConfig) + + await settings.handleResults() + + const body = getCombinedCommentBody(createComment) + expect(body).toContain('Rule B') + expect(body).not.toContain('Rule D') + expect(body).toContain('Rulesets — 1 repo, 1 policy changed') + }) + + it('uses the same table-free trimmed details in the check-run summary', async () => { + const { context, checksUpdate } = buildContext() + const result = makeNopResult({ + repo: 'my-repo', + plugin: 'labels', + additions: null, + modifications: [{ name: 'bug', color: 'blue' }] + }) + const settings = buildSettings(context, [result]) + + await settings.handleResults() + + const summary = checksUpdate.mock.calls[0][0].output.summary + expect(summary).toContain('Number of repos affected') + expect(summary).toContain('labels — 1 repo, 1 setting changed') + expect(summary).toContain('**my-repo**') + expect(summary).toContain('- `bug`') + expect(summary).toContain(' - ~ `color`') + expect(summary).toContain(' - after: `blue`') + expect(summary).not.toContain('| Repo |') + expect(summary).not.toContain('
') + }) + + it('pairs action diff entries by non-name identity fields', async () => { + const { context, createComment } = buildContext() + const result = makeNopResult({ + repo: 'my-repo', + plugin: 'teams', + deletions: [{ login: 'admin-team', permission: 'pull' }], + modifications: [{ login: 'admin-team', permission: 'push' }] + }) + const settings = buildSettings(context, [result]) + + await settings.handleResults() + + const body = getCombinedCommentBody(createComment) + expect(body).toContain('- `admin-team`') + expect(body).toContain(' - ~ `permission`') + expect(body).toContain(' - before: `pull`') + expect(body).toContain(' - after: `push`') + }) + + it('prefers structured action fields over generic msg text', async () => { + const { context, createComment } = buildContext() + const result = { + type: 'NOP', + plugin: 'labels', + repo: 'my-repo', + endpoint: '', + body: {}, + action: { + msg: 'Changes found', + additions: [{ name: 'security', color: 'red' }], + deletions: null, + modifications: null + } + } + const settings = buildSettings(context, [result]) + + await settings.handleResults() + + const body = getCombinedCommentBody(createComment) + expect(body).toContain('security') + expect(body).toContain(' - + `color`: `red`') + expect(body).not.toContain('Changes found') + }) + + it('renders large object values as compact inline JSON', async () => { + const { context, createComment } = buildContext() + const result = makeNopResult({ + repo: 'my-repo', + plugin: 'branches', + additions: [{ + name: 'main', + required_workflows: [{ path: '.github/workflows/build.yml', ref: 'main' }] + }] + }) + const settings = buildSettings(context, [result]) + + await settings.handleResults() + + const body = getCombinedCommentBody(createComment) + expect(body).toContain('- `main`') + expect(body).toContain('required_workflows') + expect(body).toContain('"path"') + expect(body).toContain('.github/workflows/build.yml') + }) + + it('shows added and deleted fields within a matched modification', async () => { + const { context, createComment } = buildContext() + const result = { + type: 'NOP', + plugin: 'labels', + repo: 'my-repo', + endpoint: '', + body: {}, + action: { + additions: null, + deletions: [{ name: 'bug', color: 'red', oldOnly: true }], + modifications: [{ name: 'bug', color: 'blue', newOnly: true }] + } + } + const settings = buildSettings(context, [result]) + + await settings.handleResults() + + const body = getCombinedCommentBody(createComment) + expect(body).toContain(' - ~ `color`') + expect(body).toContain(' - before: `red`') + expect(body).toContain(' - after: `blue`') + expect(body).toContain(' - + `newOnly`: `true`') + expect(body).toContain(' - - `oldOnly`: `true`') + }) + + it('detects nested value modifications beyond the display preview', async () => { + const { context, createComment } = buildContext() + const sharedPrefix = 'a'.repeat(240) + const result = { + type: 'NOP', + plugin: 'branches', + repo: 'my-repo', + endpoint: '', + body: {}, + action: { + additions: null, + deletions: [{ + name: 'main', + required_workflows: [{ path: `${sharedPrefix}-OLD.yml`, ref: 'main' }], + enforce_admins: false + }], + modifications: [{ + name: 'main', + required_workflows: [{ path: `${sharedPrefix}-NEW.yml`, ref: 'main' }], + enforce_admins: true + }] + } + } + const settings = buildSettings(context, [result]) + + await settings.handleResults() + + const body = getCombinedCommentBody(createComment) + expect(body).toContain(' - ~ `required_workflows`') + expect(body).toContain('-OLD.yml') + expect(body).toContain('-NEW.yml') + }) + + it('does not hide non-identity fields whose value equals the target name', async () => { + const { context, createComment } = buildContext() + const result = makeNopResult({ + repo: 'my-repo', + plugin: 'labels', + additions: [{ name: 'bug', description: 'bug', color: 'red' }] + }) + const settings = buildSettings(context, [result]) + + await settings.handleResults() + + const body = getCombinedCommentBody(createComment) + expect(body).toContain(' - + `description`: `bug`') + expect(body).toContain(' - + `color`: `red`') + }) + }) + + // ------------------------------------------------------------------------- + // Test 9 — does not create PR comment when CREATE_PR_COMMENT is not true + // ------------------------------------------------------------------------- + describe('does not create PR comment when CREATE_PR_COMMENT is not true', () => { + it('createComment is never called when CREATE_PR_COMMENT is "false"', async () => { + // Arrange + env.CREATE_PR_COMMENT = 'false' + const { context, createComment, checksUpdate } = buildContext() + const result = makeNopResult() + const settings = buildSettings(context, [result]) + + // Act + await settings.handleResults() + + // Assert + expect(createComment).not.toHaveBeenCalled() + // checks.update must still be called regardless + expect(checksUpdate).toHaveBeenCalledTimes(1) + }) + + it('createComment is never called when CREATE_PR_COMMENT is undefined', async () => { + // Arrange + env.CREATE_PR_COMMENT = undefined + const { context, createComment } = buildContext() + const result = makeNopResult() + const settings = buildSettings(context, [result]) + + // Act + await settings.handleResults() + + // Assert + expect(createComment).not.toHaveBeenCalled() + }) + }) + + // ------------------------------------------------------------------------- + // Test 9 — isDeepEmpty recursive detection + // ------------------------------------------------------------------------- + describe('isDeepEmpty recursive detection', () => { + it('null/undefined are deep-empty', () => { + expect(isDeepEmpty(null)).toBe(true) + expect(isDeepEmpty(undefined)).toBe(true) + }) + + it('empty arrays and objects are deep-empty', () => { + expect(isDeepEmpty([])).toBe(true) + expect(isDeepEmpty({})).toBe(true) + }) + + it('nested empty structures are deep-empty', () => { + expect(isDeepEmpty({ entries: [] })).toBe(true) + expect(isDeepEmpty({ a: { b: [] } })).toBe(true) + expect(isDeepEmpty({ a: null, b: undefined, c: {} })).toBe(true) + expect(isDeepEmpty([{}, [], null])).toBe(true) + }) + + it('non-empty values are NOT deep-empty', () => { + expect(isDeepEmpty('text')).toBe(false) + expect(isDeepEmpty(42)).toBe(false) + expect(isDeepEmpty(false)).toBe(false) + expect(isDeepEmpty([1])).toBe(false) + expect(isDeepEmpty({ key: 'value' })).toBe(false) + }) + + it('partially-filled nested structures are NOT deep-empty', () => { + expect(isDeepEmpty({ entries: [{ name: 'x' }] })).toBe(false) + expect(isDeepEmpty({ a: null, b: 'content' })).toBe(false) + }) + }) + + // ------------------------------------------------------------------------- + // Test 10 — isEmptyChange with deeply-nested empty structures + // ------------------------------------------------------------------------- + describe('isEmptyChange with deeply-nested empty structures', () => { + it('action with { entries: [] } additions is considered empty', () => { + expect(isEmptyChange({ additions: { entries: [] }, deletions: null, modifications: null })).toBe(true) + }) + + it('action with nested empty objects across all fields is empty', () => { + expect(isEmptyChange({ additions: { a: {} }, deletions: { b: [] }, modifications: { c: null } })).toBe(true) + }) + + it('action with real content in additions is NOT empty', () => { + expect(isEmptyChange({ additions: { entries: [{ name: 'label-a' }] }, deletions: null, modifications: null })).toBe(false) + }) + + it('filters nested-empty results from PR comment output', async () => { + const { context, createComment } = buildContext() + const nestedEmptyResult = makeNopResult({ + repo: 'nested-empty-repo', + plugin: 'labels', + additions: { entries: [] }, + deletions: { items: [{}] }, + modifications: null + }) + const settings = buildSettings(context, [nestedEmptyResult]) + + await settings.handleResults() + + const body = getCombinedCommentBody(createComment) + expect(body).not.toContain('nested-empty-repo') + expect(body).toContain('_No changes to apply._') + }) + }) + + // ------------------------------------------------------------------------- + // Test 11 — Base config filtering: org-level rulesets + // ------------------------------------------------------------------------- + describe('base config filtering for org-level rulesets', () => { + it('only shows rulesets that changed between base and PR config', async () => { + const { context, createComment } = buildContext() + + const baseConfig = { + rulesets: [ + { name: 'Rule A', enforcement: 'active', conditions: { repository_name: { include: ['*'] } } }, + { name: 'Rule B', enforcement: 'active', conditions: { repository_name: { include: ['agent-*'] } } }, + { name: 'Rule C', enforcement: 'evaluate', conditions: { repository_name: { include: ['*'] } } } + ] + } + const prConfig = { + rulesets: [ + { name: 'Rule A', enforcement: 'active', conditions: { repository_name: { include: ['*'] } } }, + { name: 'Rule B', enforcement: 'active', conditions: { repository_name: { include: ['mythapi-*'] } } }, // changed! + { name: 'Rule C', enforcement: 'evaluate', conditions: { repository_name: { include: ['*'] } } } + ] + } + + // NOP comparison found "changes" for all 3 rulesets (due to API drift) + const orgResult = { + type: 'NOP', + plugin: 'Rulesets', + repo: 'test-org (org)', + endpoint: '', + body: {}, + action: { + additions: [], + deletions: [{ name: 'Rule B', conditions: { repository_name: { include: ['agent-*'] } } }], + modifications: [ + { name: 'Rule A', bypass_actors: [{ actor_id: 1 }] }, + { name: 'Rule B', conditions: { repository_name: { include: ['mythapi-*'] } } }, + { name: 'Rule C', bypass_actors: [{ actor_id: 1 }] } + ] + } + } + + const settings = buildSettings(context, [orgResult], prConfig, baseConfig) + await settings.handleResults() + + const body = getCombinedCommentBody(createComment) + // Rule B changed — should appear + expect(body).toContain('Rule B') + // Rule A and Rule C are unchanged in config — should NOT appear + expect(body).not.toContain('Rule A') + expect(body).not.toContain('Rule C') + }) + + it('shows all rulesets when no baseConfig is provided (fallback)', async () => { + const { context, createComment } = buildContext() + + const orgResult = { + type: 'NOP', + plugin: 'Rulesets', + repo: 'test-org (org)', + endpoint: '', + body: {}, + action: { + additions: [], + deletions: [], + modifications: [ + { name: 'Rule A', bypass_actors: [{ actor_id: 1 }] }, + { name: 'Rule B', conditions: { repository_name: { include: ['mythapi-*'] } } } + ] + } + } + + // No baseConfig — should show everything (no filtering) + const settings = buildSettings(context, [orgResult], { rulesets: [] }) + await settings.handleResults() + + const body = getCombinedCommentBody(createComment) + expect(body).toContain('Rule A') + expect(body).toContain('Rule B') + }) + + it('filters out org result entirely when no rulesets changed', async () => { + const { context, createComment } = buildContext() + + const sameRulesets = [ + { name: 'Rule A', enforcement: 'active' }, + { name: 'Rule B', enforcement: 'evaluate' } + ] + const baseConfig = { rulesets: sameRulesets } + const prConfig = { rulesets: sameRulesets } + + const orgResult = { + type: 'NOP', + plugin: 'Rulesets', + repo: 'test-org (org)', + endpoint: '', + body: {}, + action: { + additions: [], + deletions: [], + modifications: [ + { name: 'Rule A', bypass_actors: [{ actor_id: 1 }] }, + { name: 'Rule B', bypass_actors: [{ actor_id: 1 }] } + ] + } + } + + const settings = buildSettings(context, [orgResult], prConfig, baseConfig) + await settings.handleResults() + + const body = getCombinedCommentBody(createComment) + expect(body).toContain('_No changes to apply._') + }) + }) + + // ------------------------------------------------------------------------- + // Test 12 — Base config filtering: repo-level results + // ------------------------------------------------------------------------- + describe('base config filtering for repo-level results', () => { + it('filters out repo results when their config section did not change', async () => { + const { context, createComment } = buildContext() + + const baseConfig = { + rulesets: [ + { name: 'Org Rule', conditions: { repository_name: { include: ['agent-*'] } } } + ], + labels: [{ name: 'bug', color: 'red' }] + } + const prConfig = { + rulesets: [ + { name: 'Org Rule', conditions: { repository_name: { include: ['mythapi-*'] } } } // changed + ], + labels: [{ name: 'bug', color: 'red' }] // unchanged + } + + // Repo-level labels result — labels section didn't change + const repoLabelsResult = makeNopResult({ + repo: 'my-repo', + plugin: 'labels', + additions: ['stale-label'] + }) + + // Org-level rulesets result — rulesets section DID change + const orgResult = { + type: 'NOP', + plugin: 'Rulesets', + repo: 'test-org (org)', + endpoint: '', + body: {}, + action: { + additions: [], + deletions: [], + modifications: [{ name: 'Org Rule', conditions: { repository_name: { include: ['mythapi-*'] } } }] + } + } + + const settings = buildSettings(context, [repoLabelsResult, orgResult], prConfig, baseConfig) + await settings.handleResults() + + const body = getCombinedCommentBody(createComment) + // Org rulesets should show + expect(body).toContain('Org Rule') + // Repo labels should NOT show (labels section unchanged) + expect(body).not.toContain('my-repo') + }) + + it('shows repo results when their config section DID change', async () => { + const { context, createComment } = buildContext() + + const baseConfig = { labels: [{ name: 'bug', color: 'red' }] } + const prConfig = { labels: [{ name: 'bug', color: 'blue' }] } // changed! + + const repoLabelsResult = makeNopResult({ + repo: 'affected-repo', + plugin: 'labels', + additions: [], + modifications: [{ name: 'bug', color: 'blue' }] + }) + + const settings = buildSettings(context, [repoLabelsResult], prConfig, baseConfig) + await settings.handleResults() + + const body = getCombinedCommentBody(createComment) + expect(body).toContain('affected-repo') + }) + + it('preserves ERROR results regardless of config filtering', async () => { + const { context, createComment } = buildContext() + + const baseConfig = { labels: [{ name: 'bug', color: 'red' }] } + const prConfig = { labels: [{ name: 'bug', color: 'red' }] } // unchanged + + const errorResult = makeErrorResult({ repo: 'error-repo', plugin: 'labels', msg: 'API failure' }) + + const settings = buildSettings(context, [errorResult], prConfig, baseConfig) + await settings.handleResults() + + const body = getCombinedCommentBody(createComment) + expect(body).toContain('error-repo') + expect(body).toContain('API failure') + }) + + it('filters out repo-level rulesets even when org rulesets section changed', async () => { + const { context, createComment } = buildContext() + + const baseConfig = { + rulesets: [{ name: 'Org Rule', conditions: { repository_name: { include: ['agent-*'] } } }] + } + const prConfig = { + rulesets: [{ name: 'Org Rule', conditions: { repository_name: { include: ['mythapi-*'] } } }] + } + + // Repo-level rulesets result (from override file, not global config) + const repoRulesetsResult = { + type: 'NOP', + plugin: 'Rulesets', + repo: 'some-repo', + endpoint: '', + body: {}, + action: { + additions: [], + deletions: [], + modifications: [{ name: 'repo-lvl-rule', enforcement: 'active' }] + } + } + + const settings = buildSettings(context, [repoRulesetsResult], prConfig, baseConfig) + await settings.handleResults() + + const body = getCombinedCommentBody(createComment) + // Repo-level rulesets should be filtered (override file didn't change) + expect(body).not.toContain('some-repo') + }) + + it('keeps repo-level results when repo is in changedRepoNames', async () => { + const { context, createComment } = buildContext() + + const baseConfig = { labels: [{ name: 'bug', color: 'red' }] } + const prConfig = { labels: [{ name: 'bug', color: 'red' }] } // unchanged globally + + // Repo-level result for a repo whose override file changed + const repoResult = makeNopResult({ + repo: 'changed-repo', + plugin: 'labels', + modifications: [{ name: 'bug', color: 'green' }] + }) + + const settings = buildSettings(context, [repoResult], prConfig, baseConfig) + // Simulate syncSelectedSettings — this repo had its override file changed + settings.changedRepoNames = new Set(['changed-repo']) + await settings.handleResults() + + const body = getCombinedCommentBody(createComment) + expect(body).toContain('changed-repo') + }) + + it('filters repo-level rulesets but keeps repo in changedRepoNames', async () => { + const { context, createComment } = buildContext() + + const baseConfig = { + rulesets: [{ name: 'Org Rule', enforcement: 'active' }] + } + const prConfig = { + rulesets: [{ name: 'Org Rule', enforcement: 'active' }] + } + + // Two repos: one selected (override changed), one not + const selectedRepoResult = { + type: 'NOP', + plugin: 'Rulesets', + repo: 'selected-repo', + endpoint: '', + body: {}, + action: { + additions: [], + deletions: [], + modifications: [{ name: 'repo-rule', enforcement: 'active' }] + } + } + const driftRepoResult = { + type: 'NOP', + plugin: 'Rulesets', + repo: 'drift-repo', + endpoint: '', + body: {}, + action: { + additions: [], + deletions: [], + modifications: [{ name: 'other-rule', enforcement: 'evaluate' }] + } + } + + const settings = buildSettings(context, [selectedRepoResult, driftRepoResult], prConfig, baseConfig) + settings.changedRepoNames = new Set(['selected-repo']) + await settings.handleResults() + + const body = getCombinedCommentBody(createComment) + expect(body).toContain('selected-repo') + expect(body).not.toContain('drift-repo') + }) + + it('keeps repo-level results for repos affected by changed suborg files', async () => { + const { context, createComment } = buildContext() + + const baseConfig = { labels: [{ name: 'bug', color: 'red' }] } + const prConfig = { labels: [{ name: 'bug', color: 'red' }] } + + const repoResult = makeNopResult({ + repo: 'suborg-repo', + plugin: 'labels', + modifications: [{ name: 'bug', color: 'green' }] + }) + + const settings = buildSettings(context, [repoResult], prConfig, baseConfig) + settings.setChangedConfigTargets([], [{ path: '.github/suborgs/engineering.yml' }]) + settings.subOrgConfigs = { + 'suborg-repo': { source: '.github/suborgs/engineering.yml' }, + 'other-repo': { source: '.github/suborgs/other.yml' } + } + + settings.trackChangedReposFromSubOrgConfigs() + await settings.handleResults() + + const body = getCombinedCommentBody(createComment) + expect(body).toContain('suborg-repo') + expect(body).not.toContain('other-repo') + }) + }) + + // ------------------------------------------------------------------------- + // Test 13 — getChangedEntryNames helper + // ------------------------------------------------------------------------- + describe('getChangedEntryNames', () => { + const { getChangedEntryNames } = require('../../../lib/settings') + + it('returns empty set when both arrays are identical', () => { + const entries = [{ name: 'A', val: 1 }, { name: 'B', val: 2 }] + expect(getChangedEntryNames(entries, entries).size).toBe(0) + }) + + it('detects added entries', () => { + const base = [{ name: 'A', val: 1 }] + const pr = [{ name: 'A', val: 1 }, { name: 'B', val: 2 }] + const changed = getChangedEntryNames(base, pr) + expect(changed.has('B')).toBe(true) + expect(changed.has('A')).toBe(false) + }) + + it('detects deleted entries', () => { + const base = [{ name: 'A', val: 1 }, { name: 'B', val: 2 }] + const pr = [{ name: 'A', val: 1 }] + const changed = getChangedEntryNames(base, pr) + expect(changed.has('B')).toBe(true) + expect(changed.has('A')).toBe(false) + }) + + it('detects modified entries', () => { + const base = [{ name: 'A', val: 1 }, { name: 'B', val: 2 }] + const pr = [{ name: 'A', val: 1 }, { name: 'B', val: 99 }] + const changed = getChangedEntryNames(base, pr) + expect(changed.has('B')).toBe(true) + expect(changed.has('A')).toBe(false) + }) + + it('handles null/undefined base gracefully', () => { + const pr = [{ name: 'A' }, { name: 'B' }] + const changed = getChangedEntryNames(null, pr) + expect(changed.has('A')).toBe(true) + expect(changed.has('B')).toBe(true) + }) + + it('handles null/undefined PR gracefully', () => { + const base = [{ name: 'A' }, { name: 'B' }] + const changed = getChangedEntryNames(base, null) + expect(changed.has('A')).toBe(true) + expect(changed.has('B')).toBe(true) + }) + }) + + // ------------------------------------------------------------------------- + // Test 14 — filterActionByChangedNames helper + // ------------------------------------------------------------------------- + describe('filterActionByChangedNames', () => { + const { filterActionByChangedNames } = require('../../../lib/settings') + + it('keeps entries matching changed names', () => { + const action = { + additions: [{ name: 'New Rule', enforcement: 'active' }], + deletions: [{ name: 'Old Rule', enforcement: 'evaluate' }], + modifications: [ + { name: 'Changed Rule', conditions: { include: ['*'] } }, + { name: 'Unchanged Rule', bypass_actors: [{ actor_id: 1 }] } + ] + } + const changed = new Set(['New Rule', 'Old Rule', 'Changed Rule']) + const result = filterActionByChangedNames(action, changed) + + expect(result.additions).toHaveLength(1) + expect(result.deletions).toHaveLength(1) + expect(result.modifications).toHaveLength(1) + expect(result.modifications[0].name).toBe('Changed Rule') + }) + + it('returns null when all entries are filtered out', () => { + const action = { + additions: [], + deletions: [], + modifications: [ + { name: 'Noise A', bypass_actors: [{ actor_id: 1 }] }, + { name: 'Noise B', bypass_actors: [{ actor_id: 2 }] } + ] + } + const changed = new Set(['Something Else']) + const result = filterActionByChangedNames(action, changed) + expect(result).toBeNull() + }) + + it('keeps entries without a name field (structural entries)', () => { + const action = { + additions: [], + deletions: [{ conditions: { repository_name: { include: ['old-*'] } } }], // no name field + modifications: [] + } + const changed = new Set(['Rule X']) + const result = filterActionByChangedNames(action, changed) + expect(result.deletions).toHaveLength(1) + }) + }) +}) diff --git a/test/unit/lib/settings.test.js b/test/unit/lib/settings.test.js index b3610651..b601757e 100644 --- a/test/unit/lib/settings.test.js +++ b/test/unit/lib/settings.test.js @@ -462,4 +462,40 @@ repository: ); }); }); + + describe('changed config tracking', () => { + it('syncAll tracks repo and suborg override changes during full nop runs', async () => { + const config = { restrictedRepos: [] } + const changedFiles = { + repos: [{ repo: 'repo-from-override' }], + subOrgs: [{ path: '.github/suborgs/frontend.yml' }] + } + + const loadConfigsSpy = jest.spyOn(Settings.prototype, 'loadConfigs').mockImplementation(async function () { + this.subOrgConfigs = { + 'repo-from-suborg': { source: '.github/suborgs/frontend.yml' }, + 'repo-from-other-suborg': { source: '.github/suborgs/backend.yml' } + } + }) + const updateOrgSpy = jest.spyOn(Settings.prototype, 'updateOrg').mockResolvedValue(undefined) + const updateAllSpy = jest.spyOn(Settings.prototype, 'updateAll').mockResolvedValue(undefined) + const handleResultsSpy = jest.spyOn(Settings.prototype, 'handleResults').mockImplementation(async function () { + expect(this.changedRepoNames.has('repo-from-override')).toBe(true) + expect(this.changedRepoNames.has('repo-from-suborg')).toBe(true) + expect(this.changedRepoNames.has('repo-from-other-suborg')).toBe(false) + }) + + await Settings.syncAll(true, stubContext, mockRepo, config, mockRef, {}, changedFiles) + + expect(loadConfigsSpy).toHaveBeenCalledTimes(1) + expect(updateOrgSpy).toHaveBeenCalledTimes(1) + expect(updateAllSpy).toHaveBeenCalledTimes(1) + expect(handleResultsSpy).toHaveBeenCalledTimes(1) + + loadConfigsSpy.mockRestore() + updateOrgSpy.mockRestore() + updateAllSpy.mockRestore() + handleResultsSpy.mockRestore() + }) + }) }) // Settings Tests