From 2876b3f2de5ee0fd8b9d7c80a4146bb0a722a543 Mon Sep 17 00:00:00 2001 From: Yadhav Jayaraman <57544838+decyjphr@users.noreply.github.com> Date: Tue, 23 Jun 2026 11:06:37 -0400 Subject: [PATCH 1/7] Fix suborg-applied settings not removed when targeting rules change When a suborg.yml file changes its targeting rules (suborgrepos, suborgteams, or suborgproperties), repos that no longer match the updated targeting were not having their suborg-applied settings (e.g. rulesets) removed. This happened because getSubOrgConfigs() only resolves the new targeting, and repos not in the new targeting were skipped in updateRepos(). Fix: Load the previous version of changed suborg config files from the base ref (payload.before for push events, pull_request.base.ref for PR/NOP mode), resolve which repos were previously targeted, compare with current targeting, and process removed repos so diffable's sync() detects and removes orphaned rulesets. Changes: - index.js: Pass payload.after/payload.before as ref/baseRef to syncSelectedSettings in push handler - lib/settings.js: Add getReposRemovedFromSubOrgTargeting() method that compares old vs new targeting to find removed repos - lib/settings.js: Add loadYamlFromRef() helper to load config from a specific git ref without cache interference - lib/settings.js: Update syncSelectedRepos to accept baseRef, identify removed repos, and process them before the suborg loop - test/unit/lib/settings.test.js: Add tests for targeting removal Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- index.js | 4 +- lib/settings.js | 123 ++++++++++++++++++++++++- test/unit/lib/settings.test.js | 160 +++++++++++++++++++++++++++++++++ 3 files changed, 284 insertions(+), 3 deletions(-) diff --git a/index.js b/index.js index 09ba59df..d3b801c5 100644 --- a/index.js +++ b/index.js @@ -100,7 +100,7 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) => } } - return Settings.syncSelectedRepos(nop, context, repos, subOrgs, config, ref, baseConfig) + return Settings.syncSelectedRepos(nop, context, repos, subOrgs, config, ref, baseConfig, baseRef) } catch (e) { if (nop) { let filename = env.SETTINGS_FILE_PATH @@ -303,7 +303,7 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) => } if (repoChanges.length > 0 || subOrgChanges.length > 0) { - return syncSelectedSettings(false, context, repoChanges, subOrgChanges) + return syncSelectedSettings(false, context, repoChanges, subOrgChanges, payload.after, payload.before) } robot.log.debug(`No changes in '${Settings.FILE_PATH}' detected, returning...`) diff --git a/lib/settings.js b/lib/settings.js index 9242044e..a9a2deb6 100644 --- a/lib/settings.js +++ b/lib/settings.js @@ -584,7 +584,7 @@ class Settings { } } - static async syncSelectedRepos (nop, context, repos, subOrgs, config, ref, baseConfig) { + static async syncSelectedRepos (nop, context, repos, subOrgs, config, ref, baseConfig, baseRef) { const settings = new Settings(nop, context, context.repo(), config, ref, null, baseConfig) settings.setChangedConfigTargets(repos, subOrgs) @@ -594,6 +594,31 @@ class Settings { settings.subOrgConfigs = await settings.getSubOrgConfigs() settings.trackChangedReposFromSubOrgConfigs() + // Identify repos removed from suborg targeting due to targeting rule + // changes in the suborg config file. These repos need processing so + // their suborg-applied settings (e.g. rulesets) are cleaned up. + if (subOrgs.length > 0 && baseRef) { + const removedRepos = await settings.getReposRemovedFromSubOrgTargeting(subOrgs, baseRef) + if (removedRepos.length > 0) { + settings.log.debug(`Repos removed from suborg targeting: ${JSON.stringify(removedRepos)}`) + // Add removed repos to changedRepoNames so NOP filtering keeps their results + if (!settings.changedRepoNames) { + settings.changedRepoNames = new Set() + } + for (const repoName of removedRepos) { + settings.changedRepoNames.add(repoName) + } + // Process removed repos with org-only config (no suborg layer) + for (const repoName of removedRepos) { + if (settings.isRestricted(repoName)) continue + if (settings.processedRepoNames.has(repoName)) continue + const repo = { owner: context.repo().owner, repo: repoName } + settings.repoConfigs = await settings.getRepoConfigs(repo) + await settings.updateRepos(repo) + } + } + } + // Re-eval is enabled only for the per-repo iteration (repo-yml change // path). The trailing suborg iteration below already iterates all suborg // repos, so it is left with the flag off. @@ -732,6 +757,102 @@ class Settings { }) } + // Identify repos that were previously targeted by suborg config files but + // are no longer targeted after the targeting rules changed. Loads the + // previous version of each changed suborg file from `baseRef`, resolves its + // targeting, and returns repo names present in the old targeting but absent + // from the current `this.subOrgConfigs`. + async getReposRemovedFromSubOrgTargeting (changedSubOrgs, baseRef) { + if (!changedSubOrgs || changedSubOrgs.length === 0 || !baseRef) { + return [] + } + + const removedRepos = [] + + for (const suborg of changedSubOrgs) { + const filePath = suborg.path + if (!filePath) continue + + // Load the previous version of this suborg config file + let previousData + try { + previousData = await this.loadYamlFromRef(filePath, baseRef) + } catch (e) { + this.log.debug(`Could not load previous suborg config from ref ${baseRef}: ${e.message}`) + continue + } + + if (!previousData) continue + + // Resolve repos targeted by the old config + const previouslyTargetedRepos = new Set() + + // 1. suborgrepos: glob patterns (these are repo name patterns) + if (previousData.suborgrepos && Array.isArray(previousData.suborgrepos)) { + for (const repoPattern of previousData.suborgrepos) { + previouslyTargetedRepos.add(repoPattern) + } + } + + // 2. suborgteams: resolve via GitHub API (team membership is live state) + if (previousData.suborgteams && Array.isArray(previousData.suborgteams)) { + try { + const teamPromises = previousData.suborgteams.map(teamslug => + this.getReposForTeam(teamslug) + ) + const teamResults = await Promise.all(teamPromises) + for (const repos of teamResults) { + for (const repo of repos) { + previouslyTargetedRepos.add(repo.name) + } + } + } catch (e) { + this.log.debug(`Error resolving previous suborgteams: ${e.message}`) + } + } + + // 3. suborgproperties: resolve via GitHub API (property values are live state) + if (previousData.suborgproperties && Array.isArray(previousData.suborgproperties)) { + try { + const subOrgRepositories = await this.getSubOrgRepositories(previousData.suborgproperties) + for (const repo of subOrgRepositories) { + previouslyTargetedRepos.add(repo.repository_name) + } + } catch (e) { + this.log.debug(`Error resolving previous suborgproperties: ${e.message}`) + } + } + + // Find repos in previous targeting that are NOT in current targeting + for (const repoName of previouslyTargetedRepos) { + if (!this.getSubOrgConfig(repoName)) { + removedRepos.push(repoName) + } + } + } + + return [...new Set(removedRepos)] + } + + // Load a YAML file from a specific git ref, bypassing the file cache. + // Used to load previous versions of config files for comparison. + async loadYamlFromRef (filePath, ref) { + const repo = { owner: this.repo.owner, repo: env.ADMIN_REPO } + const params = Object.assign(repo, { path: filePath, ref }) + + const response = await this.github.repos.getContent(params) + + if (Array.isArray(response.data)) { + return null + } + + if (typeof response.data.content !== 'string') { + return null + } + + return yaml.load(Buffer.from(response.data.content, 'base64').toString()) || {} + } + // Create a check in the Admin repo for safe-settings. async createCheckRun () { const startTime = new Date() diff --git a/test/unit/lib/settings.test.js b/test/unit/lib/settings.test.js index 14660189..06c0a4d6 100644 --- a/test/unit/lib/settings.test.js +++ b/test/unit/lib/settings.test.js @@ -1406,4 +1406,164 @@ repository: }) }) }) + + describe('getReposRemovedFromSubOrgTargeting', () => { + let settings + + beforeEach(() => { + stubConfig = { restrictedRepos: {} } + settings = createSettings(stubConfig) + }) + + it('returns empty array when no changedSubOrgs provided', async () => { + const result = await settings.getReposRemovedFromSubOrgTargeting([], 'prev-sha') + expect(result).toEqual([]) + }) + + it('returns empty array when no baseRef provided', async () => { + const result = await settings.getReposRemovedFromSubOrgTargeting([{ path: '.github/suborgs/frontend.yml' }], null) + expect(result).toEqual([]) + }) + + it('identifies repos removed from suborgrepos targeting', async () => { + // Previous config had repo-a and repo-b in suborgrepos + const previousContent = Buffer.from(yaml.dump({ + suborgrepos: ['repo-a', 'repo-b'], + teams: [{ name: 'core', permission: 'push' }] + })).toString('base64') + + stubContext.octokit.repos.getContent = jest.fn().mockImplementation((params) => { + if (params.ref === 'prev-sha') { + return Promise.resolve({ data: { content: previousContent } }) + } + // Current config: default mock (has new-repo in suborgrepos) + const currentContent = Buffer.from(yaml.dump({ + suborgrepos: ['repo-b'], + teams: [{ name: 'core', permission: 'push' }] + })).toString('base64') + return Promise.resolve({ data: { content: currentContent } }) + }) + + // Current subOrgConfigs only has repo-b (repo-a was removed from targeting) + settings.subOrgConfigs = { + 'repo-b': { source: '.github/suborgs/frontend.yml' } + } + + const result = await settings.getReposRemovedFromSubOrgTargeting( + [{ path: '.github/suborgs/frontend.yml', name: 'frontend' }], + 'prev-sha' + ) + + expect(result).toContain('repo-a') + expect(result).not.toContain('repo-b') + }) + + it('identifies repos removed from suborgteams targeting', async () => { + // Previous config used suborgteams: [team-a] + const previousContent = Buffer.from(yaml.dump({ + suborgteams: ['team-a'], + teams: [{ name: 'core', permission: 'push' }] + })).toString('base64') + + stubContext.octokit.repos.getContent = jest.fn().mockImplementation((params) => { + if (params.ref === 'prev-sha') { + return Promise.resolve({ data: { content: previousContent } }) + } + return Promise.resolve({ data: { content: previousContent } }) + }) + + // Mock getReposForTeam to return repos for team-a + settings.getReposForTeam = jest.fn().mockResolvedValue([ + { name: 'team-repo-1' }, + { name: 'team-repo-2' } + ]) + + // Current subOrgConfigs: only team-repo-1 still matches (team-repo-2 was removed) + settings.subOrgConfigs = { + 'team-repo-1': { source: '.github/suborgs/frontend.yml' } + } + + const result = await settings.getReposRemovedFromSubOrgTargeting( + [{ path: '.github/suborgs/frontend.yml', name: 'frontend' }], + 'prev-sha' + ) + + expect(result).toContain('team-repo-2') + expect(result).not.toContain('team-repo-1') + }) + + it('identifies repos removed from suborgproperties targeting', async () => { + // Previous config used suborgproperties + const previousContent = Buffer.from(yaml.dump({ + suborgproperties: [{ EDP: true }], + teams: [{ name: 'core', permission: 'push' }] + })).toString('base64') + + stubContext.octokit.repos.getContent = jest.fn().mockImplementation((params) => { + if (params.ref === 'prev-sha') { + return Promise.resolve({ data: { content: previousContent } }) + } + return Promise.resolve({ data: { content: previousContent } }) + }) + + // Mock getSubOrgRepositories to return repos with the property + settings.getSubOrgRepositories = jest.fn().mockResolvedValue([ + { repository_name: 'prop-repo-1' }, + { repository_name: 'prop-repo-2' } + ]) + + // Current subOrgConfigs: only prop-repo-1 still matches + settings.subOrgConfigs = { + 'prop-repo-1': { source: '.github/suborgs/frontend.yml' } + } + + const result = await settings.getReposRemovedFromSubOrgTargeting( + [{ path: '.github/suborgs/frontend.yml', name: 'frontend' }], + 'prev-sha' + ) + + expect(result).toContain('prop-repo-2') + expect(result).not.toContain('prop-repo-1') + }) + + it('deduplicates removed repos across multiple suborg files', async () => { + const previousContent = Buffer.from(yaml.dump({ + suborgrepos: ['repo-a', 'repo-b'] + })).toString('base64') + + stubContext.octokit.repos.getContent = jest.fn().mockResolvedValue({ + data: { content: previousContent } + }) + + // Neither repo matches current targeting + settings.subOrgConfigs = {} + + const result = await settings.getReposRemovedFromSubOrgTargeting( + [ + { path: '.github/suborgs/frontend.yml', name: 'frontend' }, + { path: '.github/suborgs/frontend.yml', name: 'frontend' } // duplicate + ], + 'prev-sha' + ) + + // Should be deduplicated + const repoACount = result.filter(r => r === 'repo-a').length + expect(repoACount).toBe(1) + }) + + it('handles 404 gracefully when previous file does not exist', async () => { + stubContext.octokit.repos.getContent = jest.fn().mockRejectedValue( + Object.assign(new Error('Not Found'), { status: 404 }) + ) + + settings.subOrgConfigs = {} + + const result = await settings.getReposRemovedFromSubOrgTargeting( + [{ path: '.github/suborgs/new-suborg.yml', name: 'new-suborg' }], + 'prev-sha' + ) + + expect(result).toEqual([]) + }) + }) }) // Settings Tests From 8659138a5c00c46bd04fbf39d53acc09a7d63667 Mon Sep 17 00:00:00 2001 From: Yadhav Jayaraman <57544838+decyjphr@users.noreply.github.com> Date: Tue, 23 Jun 2026 11:30:47 -0400 Subject: [PATCH 2/7] Add phase 5 smoke test for suborg targeting rule change removal Adds a sub-test to phase 5 that narrows suborg targeting from suborgteams to suborgrepos (excluding demo-repo-service1), then verifies the suborg ruleset is removed from the dropped repo while retained on the still-targeted repo. Restores team-targeted config afterward for subsequent phases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- smoke-test.js | 61 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/smoke-test.js b/smoke-test.js index 00ca7d30..27013b4e 100644 --- a/smoke-test.js +++ b/smoke-test.js @@ -644,6 +644,15 @@ const SUBORG_EXPERT_SERVICES_PROPERTY_YML = SUBORG_EXPERT_SERVICES_YML.replace( 'suborgproperties:\n - ent-ownership: expert-services' ) +// Suborg config that narrows targeting to only demo-repo-service2 via +// suborgrepos. Used to test that repos dropping out of suborg targeting +// (due to targeting rule changes in the suborg.yml) have their +// suborg-applied rulesets removed. +const SUBORG_EXPERT_SERVICES_NARROW_YML = SUBORG_EXPERT_SERVICES_YML.replace( + 'suborgteams:\n - expert-services-developers', + 'suborgrepos:\n - demo-repo-service2' +) + const REPO_DEMO_SERVICE1_ARCHIVED_YML = `# Safe-Settings Configuration repository: name: demo-repo-service1 @@ -1404,9 +1413,61 @@ async function phase5Suborg () { if (!await safeMerge(ORG, ADMIN_REPO, pr3.number)) return await sleep(WEBHOOK_SETTLE_MS) + // ── Sub-test: suborg targeting rule change removes rulesets from dropped repos ── + log('Verifying suborg ruleset is on demo-repo-service1 before targeting change...') + const demo1RulesetBeforeNarrow = await poll(async () => { + return await getRepoRuleset(ORG, 'demo-repo-service1', suborgRulesetName) + }, { desc: 'suborg ruleset on demo-repo-service1 before narrowing', timeout: 90000 }) + assert(demo1RulesetBeforeNarrow !== null, 'Suborg ruleset present on demo-repo-service1 before targeting change') + + const branch4 = 'smoke-test-phase5-narrow-targeting' + await deleteBranch(ORG, ADMIN_REPO, branch4) + await createBranch(ORG, ADMIN_REPO, branch4) + await createOrUpdateFile(ORG, ADMIN_REPO, `${CONFIG_PATH}/suborgs/expert-services.yml`, SUBORG_EXPERT_SERVICES_NARROW_YML, branch4, 'Narrow suborg targeting to only demo-repo-service2') + + const pr4 = await createPR(ORG, ADMIN_REPO, 'Smoke test: narrow suborg targeting (remove demo-repo-service1)', branch4, defaultBranch) + log('Waiting for NOP check run on narrowed targeting...') + await sleep(WEBHOOK_SETTLE_MS) + const checkRun4 = await waitForCheckRun(ORG, ADMIN_REPO, pr4.head.sha) + assert(checkRun4 !== null, 'Check run completed for narrowed targeting') + if (checkRun4) assert(checkRun4.conclusion === 'success', `Check run conclusion is success (got: ${checkRun4.conclusion})`) + + if (!await safeMerge(ORG, ADMIN_REPO, pr4.number)) return + await sleep(WEBHOOK_SETTLE_MS + 15000) + + log('Checking suborg ruleset removed from demo-repo-service1 after targeting change...') + const demo1RulesetAfterNarrow = await poll(async () => { + const ruleset = await getRepoRuleset(ORG, 'demo-repo-service1', suborgRulesetName) + return ruleset === null ? true : null + }, { desc: 'suborg ruleset to be removed from demo-repo-service1 after targeting narrowed', timeout: 90000 }) + assert(demo1RulesetAfterNarrow === true, 'Suborg ruleset removed from demo-repo-service1 after targeting rule change') + + const demo2RulesetAfterNarrow = await poll(async () => { + return await getRepoRuleset(ORG, 'demo-repo-service2', suborgRulesetName) + }, { desc: 'suborg ruleset retained on demo-repo-service2 after narrowing', timeout: 60000 }) + assert(demo2RulesetAfterNarrow !== null, 'Suborg ruleset retained on demo-repo-service2 after targeting rule change') + + // Restore team-targeted config for subsequent phases + const branch5 = 'smoke-test-phase5-restore-after-narrow' + await deleteBranch(ORG, ADMIN_REPO, branch5) + await createBranch(ORG, ADMIN_REPO, branch5) + await createOrUpdateFile(ORG, ADMIN_REPO, `${CONFIG_PATH}/suborgs/expert-services.yml`, SUBORG_EXPERT_SERVICES_YML, branch5, 'Restore team-targeted expert-services suborg config after narrow test') + + const pr5 = await createPR(ORG, ADMIN_REPO, 'Smoke test: restore suborg after narrow targeting test', branch5, defaultBranch) + log('Waiting for NOP check run on restore...') + await sleep(WEBHOOK_SETTLE_MS) + const checkRun5 = await waitForCheckRun(ORG, ADMIN_REPO, pr5.head.sha) + assert(checkRun5 !== null, 'Check run completed for restore after narrow') + if (checkRun5) assert(checkRun5.conclusion === 'success', `Check run conclusion is success (got: ${checkRun5.conclusion})`) + + if (!await safeMerge(ORG, ADMIN_REPO, pr5.number)) return + await sleep(WEBHOOK_SETTLE_MS) + await deleteBranch(ORG, ADMIN_REPO, branch) await deleteBranch(ORG, ADMIN_REPO, branch2) await deleteBranch(ORG, ADMIN_REPO, branch3) + await deleteBranch(ORG, ADMIN_REPO, branch4) + await deleteBranch(ORG, ADMIN_REPO, branch5) } async function phase6Archive () { From 90f0dea8649042ef740683ed938c98688f9ad020 Mon Sep 17 00:00:00 2001 From: Yadhav Jayaraman <57544838+decyjphr@users.noreply.github.com> Date: Tue, 23 Jun 2026 12:08:41 -0400 Subject: [PATCH 3/7] Fix custom_properties test to use github.rest.repos mock The plugin was updated to use github.rest.repos.* but the test was still mocking github.repos.*, causing TypeError failures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../lib/plugins/custom_properties.test.js | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/test/unit/lib/plugins/custom_properties.test.js b/test/unit/lib/plugins/custom_properties.test.js index 429544f8..6afc8228 100644 --- a/test/unit/lib/plugins/custom_properties.test.js +++ b/test/unit/lib/plugins/custom_properties.test.js @@ -15,9 +15,11 @@ describe('CustomProperties', () => { beforeEach(() => { github = { paginate: jest.fn(), - repos: { - getCustomPropertiesValues: jest.fn(), - createOrUpdateCustomPropertiesValues: jest.fn() + rest: { + repos: { + getCustomPropertiesValues: jest.fn(), + createOrUpdateCustomPropertiesValues: jest.fn() + } } } @@ -42,7 +44,7 @@ describe('CustomProperties', () => { const result = await plugin.find() expect(github.paginate).toHaveBeenCalledWith( - github.repos.getCustomPropertiesValues, + github.rest.repos.getCustomPropertiesValues, { owner, repo, @@ -75,14 +77,14 @@ describe('CustomProperties', () => { return plugin.sync().then(() => { expect(github.paginate).toHaveBeenCalledWith( - github.repos.getCustomPropertiesValues, + github.rest.repos.getCustomPropertiesValues, { owner, repo, per_page: 100 } ) - expect(github.repos.createOrUpdateCustomPropertiesValues).not.toHaveBeenCalledWith({ + expect(github.rest.repos.createOrUpdateCustomPropertiesValues).not.toHaveBeenCalledWith({ owner, repo, properties: [ @@ -92,7 +94,7 @@ describe('CustomProperties', () => { } ] }) - expect(github.repos.createOrUpdateCustomPropertiesValues).toHaveBeenCalledWith({ + expect(github.rest.repos.createOrUpdateCustomPropertiesValues).toHaveBeenCalledWith({ owner, repo, properties: [ @@ -102,7 +104,7 @@ describe('CustomProperties', () => { } ] }) - expect(github.repos.createOrUpdateCustomPropertiesValues).toHaveBeenCalledWith({ + expect(github.rest.repos.createOrUpdateCustomPropertiesValues).toHaveBeenCalledWith({ owner, repo, properties: [ @@ -112,7 +114,7 @@ describe('CustomProperties', () => { } ] }) - expect(github.repos.createOrUpdateCustomPropertiesValues).toHaveBeenCalledWith({ + expect(github.rest.repos.createOrUpdateCustomPropertiesValues).toHaveBeenCalledWith({ owner, repo, properties: [ @@ -127,7 +129,7 @@ describe('CustomProperties', () => { // const plugin = configure([{ name: 'Test', value: 'test' }]) // await plugin.update({ name: 'test', value: 'old' }, { name: 'test', value: 'test' }) - // expect(github.repos.createOrUpdateCustomPropertiesValues).toHaveBeenCalledWith({ + // expect(github.rest.repos.createOrUpdateCustomPropertiesValues).toHaveBeenCalledWith({ // owner, // repo, // properties: [ From 30c514a52f0320c147eba07ca67784a1aa314c8c Mon Sep 17 00:00:00 2001 From: Yadhav Jayaraman <57544838+decyjphr@users.noreply.github.com> Date: Tue, 23 Jun 2026 12:50:55 -0400 Subject: [PATCH 4/7] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- lib/settings.js | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/settings.js b/lib/settings.js index a9a2deb6..655154eb 100644 --- a/lib/settings.js +++ b/lib/settings.js @@ -787,10 +787,16 @@ class Settings { // Resolve repos targeted by the old config const previouslyTargetedRepos = new Set() - // 1. suborgrepos: glob patterns (these are repo name patterns) + // 1. suborgrepos: resolve glob patterns to concrete repo names if (previousData.suborgrepos && Array.isArray(previousData.suborgrepos)) { + const allRepos = await this.github.paginate('GET /installation/repositories') for (const repoPattern of previousData.suborgrepos) { - previouslyTargetedRepos.add(repoPattern) + const glob = new Glob(repoPattern) + for (const repo of allRepos) { + if (glob.test(repo.name)) { + previouslyTargetedRepos.add(repo.name) + } + } } } From 6d9e38c64d22c222a8cd47c7fad7384fa5eb1aa3 Mon Sep 17 00:00:00 2001 From: Yadhav Jayaraman <57544838+decyjphr@users.noreply.github.com> Date: Tue, 23 Jun 2026 12:51:22 -0400 Subject: [PATCH 5/7] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- test/unit/lib/settings.test.js | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/test/unit/lib/settings.test.js b/test/unit/lib/settings.test.js index 06c0a4d6..1ab9c92c 100644 --- a/test/unit/lib/settings.test.js +++ b/test/unit/lib/settings.test.js @@ -1458,6 +1458,40 @@ repository: expect(result).not.toContain('repo-b') }) + it('identifies repos removed from suborgrepos glob targeting', async () => { + const previousContent = Buffer.from(yaml.dump({ + suborgrepos: ['team-*'], + teams: [{ name: 'core', permission: 'push' }] + })).toString('base64') + + stubContext.octokit.repos.getContent = jest.fn().mockImplementation((params) => { + if (params.ref === 'prev-sha') { + return Promise.resolve({ data: { content: previousContent } }) + } + return Promise.resolve({ data: { content: previousContent } }) + }) + + stubContext.octokit.paginate = jest.fn().mockResolvedValue([ + { owner: { login: 'test' }, name: 'team-a1' }, + { owner: { login: 'test' }, name: 'team-b1' }, + { owner: { login: 'test' }, name: 'other' } + ]) + + // Current targeting only matches team-a* + settings.subOrgConfigs = { + 'team-a*': { source: '.github/suborgs/frontend.yml' } + } + + const result = await settings.getReposRemovedFromSubOrgTargeting( + [{ path: '.github/suborgs/frontend.yml', name: 'frontend' }], + 'prev-sha' + ) + + expect(result).toContain('team-b1') + expect(result).not.toContain('team-a1') + expect(result).not.toContain('other') + }) + it('identifies repos removed from suborgteams targeting', async () => { // Previous config used suborgteams: [team-a] const previousContent = Buffer.from(yaml.dump({ From 9b49716a210b5bd9822e758ab030fce47c1eea7c Mon Sep 17 00:00:00 2001 From: Yadhav Jayaraman <57544838+decyjphr@users.noreply.github.com> Date: Tue, 23 Jun 2026 12:58:21 -0400 Subject: [PATCH 6/7] Fix suborgrepos tests to mock paginate for glob resolution The implementation now resolves suborgrepos glob patterns against installation repos via github.paginate(). Updated the tests to mock paginate returning the repos that match the patterns. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- test/unit/lib/settings.test.js | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/unit/lib/settings.test.js b/test/unit/lib/settings.test.js index 1ab9c92c..74de9d7e 100644 --- a/test/unit/lib/settings.test.js +++ b/test/unit/lib/settings.test.js @@ -1444,6 +1444,12 @@ repository: return Promise.resolve({ data: { content: currentContent } }) }) + // Mock installation repos for glob resolution + stubContext.octokit.paginate = jest.fn().mockResolvedValue([ + { name: 'repo-a', owner: { login: 'test' } }, + { name: 'repo-b', owner: { login: 'test' } } + ]) + // Current subOrgConfigs only has repo-b (repo-a was removed from targeting) settings.subOrgConfigs = { 'repo-b': { source: '.github/suborgs/frontend.yml' } @@ -1569,6 +1575,12 @@ repository: data: { content: previousContent } }) + // Mock installation repos for glob resolution + stubContext.octokit.paginate = jest.fn().mockResolvedValue([ + { name: 'repo-a', owner: { login: 'test' } }, + { name: 'repo-b', owner: { login: 'test' } } + ]) + // Neither repo matches current targeting settings.subOrgConfigs = {} From ad373e4ca07c2e4f58514944d302cdc4d15cde03 Mon Sep 17 00:00:00 2001 From: Yadhav Jayaraman <57544838+decyjphr@users.noreply.github.com> Date: Tue, 23 Jun 2026 14:07:07 -0400 Subject: [PATCH 7/7] Fix suborg targeting removal: inject empty plugin sections for cleanup When repos are removed from suborg targeting, the rulesets plugin was never instantiated because returnRepoSpecificConfigs strips 'rulesets' from org config, and the repo has no suborg/override rulesets section. Without the plugin, existing suborg-applied rulesets were never removed. Fix: Track which plugin sections the previous suborg config provided (e.g. rulesets, teams) and inject them as empty arrays into the merged config for removed repos. This ensures the plugins are instantiated and sync() detects/removes existing entries. Also fix smoke test to use 'test' repo (exists in Phase 1) instead of 'demo-repo-service2' (not created until Phase 7) for the retention assertion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- lib/settings.js | 45 +++++++++++++++++++++++++++++++--- smoke-test.js | 14 +++++------ test/unit/lib/settings.test.js | 33 +++++++++++++------------ 3 files changed, 66 insertions(+), 26 deletions(-) diff --git a/lib/settings.js b/lib/settings.js index 655154eb..3a10e4fc 100644 --- a/lib/settings.js +++ b/lib/settings.js @@ -598,9 +598,12 @@ class Settings { // changes in the suborg config file. These repos need processing so // their suborg-applied settings (e.g. rulesets) are cleaned up. if (subOrgs.length > 0 && baseRef) { - const removedRepos = await settings.getReposRemovedFromSubOrgTargeting(subOrgs, baseRef) + const removalResult = await settings.getReposRemovedFromSubOrgTargeting(subOrgs, baseRef) + const removedRepos = removalResult.repos + const previousPluginSections = removalResult.previousPluginSections if (removedRepos.length > 0) { settings.log.debug(`Repos removed from suborg targeting: ${JSON.stringify(removedRepos)}`) + settings.log.debug(`Previous suborg plugin sections to clean up: ${JSON.stringify(previousPluginSections)}`) // Add removed repos to changedRepoNames so NOP filtering keeps their results if (!settings.changedRepoNames) { settings.changedRepoNames = new Set() @@ -608,7 +611,11 @@ class Settings { for (const repoName of removedRepos) { settings.changedRepoNames.add(repoName) } - // Process removed repos with org-only config (no suborg layer) + // Process removed repos with org-only config (no suborg layer). + // Inject empty arrays for plugin sections that were in the previous + // suborg config so the plugins are instantiated and can detect/remove + // existing entries that are no longer desired. + settings.removedFromSubOrgPluginSections = previousPluginSections for (const repoName of removedRepos) { if (settings.isRestricted(repoName)) continue if (settings.processedRepoNames.has(repoName)) continue @@ -616,6 +623,7 @@ class Settings { settings.repoConfigs = await settings.getRepoConfigs(repo) await settings.updateRepos(repo) } + settings.removedFromSubOrgPluginSections = null } } @@ -763,11 +771,13 @@ class Settings { // targeting, and returns repo names present in the old targeting but absent // from the current `this.subOrgConfigs`. async getReposRemovedFromSubOrgTargeting (changedSubOrgs, baseRef) { + const emptyResult = { repos: [], previousPluginSections: [] } if (!changedSubOrgs || changedSubOrgs.length === 0 || !baseRef) { - return [] + return emptyResult } const removedRepos = [] + let previousPluginSections = null for (const suborg of changedSubOrgs) { const filePath = suborg.path @@ -835,9 +845,24 @@ class Settings { removedRepos.push(repoName) } } + + // Collect plugin sections from previous config that need cleanup + // (these are sections that were applied by the suborg and need to be + // synced with empty config so existing entries are removed) + if (!previousPluginSections) { + previousPluginSections = new Set() + } + for (const key of Object.keys(previousData)) { + if (key in Settings.PLUGINS) { + previousPluginSections.add(key) + } + } } - return [...new Set(removedRepos)] + return { + repos: [...new Set(removedRepos)], + previousPluginSections: previousPluginSections ? [...previousPluginSections] : [] + } } // Load a YAML file from a specific git ref, bypassing the file cache. @@ -1708,6 +1733,18 @@ class Settings { const overrideConfig = this.mergeDeep.mergeDeep({}, sources.org, sources.suborg, sources.repo) + // When processing repos removed from suborg targeting, inject empty arrays + // for plugin sections that were previously provided by the suborg. This + // ensures those plugins are instantiated and can detect/remove existing + // entries that are no longer desired. + if (this.removedFromSubOrgPluginSections && !subOrgOverrideConfig) { + for (const section of this.removedFromSubOrgPluginSections) { + if (!(section in overrideConfig)) { + overrideConfig[section] = [] + } + } + } + this.log.debug(`consolidated config is ${JSON.stringify(overrideConfig)}`) const childPlugins = [] diff --git a/smoke-test.js b/smoke-test.js index 27013b4e..40b705dc 100644 --- a/smoke-test.js +++ b/smoke-test.js @@ -644,13 +644,13 @@ const SUBORG_EXPERT_SERVICES_PROPERTY_YML = SUBORG_EXPERT_SERVICES_YML.replace( 'suborgproperties:\n - ent-ownership: expert-services' ) -// Suborg config that narrows targeting to only demo-repo-service2 via +// Suborg config that narrows targeting to only the 'test' repo via // suborgrepos. Used to test that repos dropping out of suborg targeting // (due to targeting rule changes in the suborg.yml) have their // suborg-applied rulesets removed. const SUBORG_EXPERT_SERVICES_NARROW_YML = SUBORG_EXPERT_SERVICES_YML.replace( 'suborgteams:\n - expert-services-developers', - 'suborgrepos:\n - demo-repo-service2' + 'suborgrepos:\n - test' ) const REPO_DEMO_SERVICE1_ARCHIVED_YML = `# Safe-Settings Configuration @@ -1423,7 +1423,7 @@ async function phase5Suborg () { const branch4 = 'smoke-test-phase5-narrow-targeting' await deleteBranch(ORG, ADMIN_REPO, branch4) await createBranch(ORG, ADMIN_REPO, branch4) - await createOrUpdateFile(ORG, ADMIN_REPO, `${CONFIG_PATH}/suborgs/expert-services.yml`, SUBORG_EXPERT_SERVICES_NARROW_YML, branch4, 'Narrow suborg targeting to only demo-repo-service2') + await createOrUpdateFile(ORG, ADMIN_REPO, `${CONFIG_PATH}/suborgs/expert-services.yml`, SUBORG_EXPERT_SERVICES_NARROW_YML, branch4, 'Narrow suborg targeting to only test repo') const pr4 = await createPR(ORG, ADMIN_REPO, 'Smoke test: narrow suborg targeting (remove demo-repo-service1)', branch4, defaultBranch) log('Waiting for NOP check run on narrowed targeting...') @@ -1442,10 +1442,10 @@ async function phase5Suborg () { }, { desc: 'suborg ruleset to be removed from demo-repo-service1 after targeting narrowed', timeout: 90000 }) assert(demo1RulesetAfterNarrow === true, 'Suborg ruleset removed from demo-repo-service1 after targeting rule change') - const demo2RulesetAfterNarrow = await poll(async () => { - return await getRepoRuleset(ORG, 'demo-repo-service2', suborgRulesetName) - }, { desc: 'suborg ruleset retained on demo-repo-service2 after narrowing', timeout: 60000 }) - assert(demo2RulesetAfterNarrow !== null, 'Suborg ruleset retained on demo-repo-service2 after targeting rule change') + const testRulesetAfterNarrow = await poll(async () => { + return await getRepoRuleset(ORG, 'test', suborgRulesetName) + }, { desc: 'suborg ruleset retained on test after narrowing', timeout: 60000 }) + assert(testRulesetAfterNarrow !== null, 'Suborg ruleset retained on test after targeting rule change') // Restore team-targeted config for subsequent phases const branch5 = 'smoke-test-phase5-restore-after-narrow' diff --git a/test/unit/lib/settings.test.js b/test/unit/lib/settings.test.js index 74de9d7e..79d4a8a1 100644 --- a/test/unit/lib/settings.test.js +++ b/test/unit/lib/settings.test.js @@ -1415,14 +1415,16 @@ repository: settings = createSettings(stubConfig) }) - it('returns empty array when no changedSubOrgs provided', async () => { + it('returns empty result when no changedSubOrgs provided', async () => { const result = await settings.getReposRemovedFromSubOrgTargeting([], 'prev-sha') - expect(result).toEqual([]) + expect(result.repos).toEqual([]) + expect(result.previousPluginSections).toEqual([]) }) - it('returns empty array when no baseRef provided', async () => { + it('returns empty result when no baseRef provided', async () => { const result = await settings.getReposRemovedFromSubOrgTargeting([{ path: '.github/suborgs/frontend.yml' }], null) - expect(result).toEqual([]) + expect(result.repos).toEqual([]) + expect(result.previousPluginSections).toEqual([]) }) it('identifies repos removed from suborgrepos targeting', async () => { @@ -1460,8 +1462,9 @@ repository: 'prev-sha' ) - expect(result).toContain('repo-a') - expect(result).not.toContain('repo-b') + expect(result.repos).toContain('repo-a') + expect(result.repos).not.toContain('repo-b') + expect(result.previousPluginSections).toContain('teams') }) it('identifies repos removed from suborgrepos glob targeting', async () => { @@ -1493,9 +1496,9 @@ repository: 'prev-sha' ) - expect(result).toContain('team-b1') - expect(result).not.toContain('team-a1') - expect(result).not.toContain('other') + expect(result.repos).toContain('team-b1') + expect(result.repos).not.toContain('team-a1') + expect(result.repos).not.toContain('other') }) it('identifies repos removed from suborgteams targeting', async () => { @@ -1528,8 +1531,8 @@ repository: 'prev-sha' ) - expect(result).toContain('team-repo-2') - expect(result).not.toContain('team-repo-1') + expect(result.repos).toContain('team-repo-2') + expect(result.repos).not.toContain('team-repo-1') }) it('identifies repos removed from suborgproperties targeting', async () => { @@ -1562,8 +1565,8 @@ repository: 'prev-sha' ) - expect(result).toContain('prop-repo-2') - expect(result).not.toContain('prop-repo-1') + expect(result.repos).toContain('prop-repo-2') + expect(result.repos).not.toContain('prop-repo-1') }) it('deduplicates removed repos across multiple suborg files', async () => { @@ -1593,7 +1596,7 @@ repository: ) // Should be deduplicated - const repoACount = result.filter(r => r === 'repo-a').length + const repoACount = result.repos.filter(r => r === 'repo-a').length expect(repoACount).toBe(1) }) @@ -1609,7 +1612,7 @@ repository: 'prev-sha' ) - expect(result).toEqual([]) + expect(result).toEqual({ repos: [], previousPluginSections: [] }) }) }) }) // Settings Tests