From 9d596ec83a73b5967255937f8ea2ee53206b2f4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomas=20Daba=C5=A1inskas?= Date: Sat, 11 Jul 2026 14:00:14 +0300 Subject: [PATCH 1/2] fix(branches): prevent duplicate nop entries for multiple branches When running in nop mode with multiple branches configured, the plugin was accumulating NopCommand entries in a shared `resArray` across all branches, causing each branch's planned changes to be duplicated for every subsequent branch processed. Moved the results array from module scope to branch scope so each branch collects its own NopCommand entries independently. Also updated the deduplication logic in `handleResults` to compare the full NopCommand object (endpoint, body, and action) rather than just type/repo/plugin, which was incorrectly treating distinct branch protection changes as duplicates. --- lib/plugins/branches.js | 22 ++++++++--------- lib/settings.js | 8 ++++-- test/unit/lib/plugins/branches.test.js | 33 +++++++++++++++++++++++++ test/unit/lib/settings.test.js | 34 ++++++++++++++++++++++++++ 4 files changed, 84 insertions(+), 13 deletions(-) diff --git a/lib/plugins/branches.js b/lib/plugins/branches.js index d60a20cc3..6ba08796c 100644 --- a/lib/plugins/branches.js +++ b/lib/plugins/branches.js @@ -30,12 +30,12 @@ module.exports = class Branches extends ErrorStash { } sync () { - const resArray = [] return this.github.rest.repos.get(this.repo).then((currentRepo) => { return Promise.all( this.branches .filter(branch => branch.protection !== undefined) .map(branch => { + const branchResults = [] // If branch protection is empty if (this.isEmpty(branch.protection)) { let p = Object.assign(this.repo, { branch: branch.name }) @@ -46,10 +46,10 @@ module.exports = class Branches extends ErrorStash { // Hack to handle closures and keep params from changing const params = Object.assign({}, p) if (this.nop) { - resArray.push( + branchResults.push( new NopCommand(this.constructor.name, this.repo, this.github.rest.repos.deleteBranchProtection.endpoint(params), 'Delete Branch Protection') ) - return Promise.resolve(resArray) + return Promise.resolve(branchResults) } return this.github.rest.repos.deleteBranchProtection(params).catch(e => { return [] }) @@ -71,21 +71,21 @@ module.exports = class Branches extends ErrorStash { if (!changes.hasChanges) { this.log.debug(`There are no changes for branch ${JSON.stringify(params)}. Skipping branch protection changes`) if (this.nop) { - return Promise.resolve(resArray) + return Promise.resolve(branchResults) } return Promise.resolve() } this.log.debug(`There are changes for branch ${JSON.stringify(params)}\n ${JSON.stringify(changes)} \n Branch protection will be applied`) if (this.nop) { - resArray.push(new NopCommand(this.constructor.name, this.repo, null, results)) + branchResults.push(new NopCommand(this.constructor.name, this.repo, null, results)) } Object.assign(params, requiredBranchProtectionDefaults, this.reformatAndReturnBranchProtection(structuredClone(result.data)), Overrides.removeOverrides(overrides, branch.protection, result.data), { headers: previewHeaders }) if (this.nop) { - resArray.push(new NopCommand(this.constructor.name, this.repo, this.github.rest.repos.updateBranchProtection.endpoint(params), 'Add Branch Protection')) - return Promise.resolve(resArray) + branchResults.push(new NopCommand(this.constructor.name, this.repo, this.github.rest.repos.updateBranchProtection.endpoint(params), 'Add Branch Protection')) + return Promise.resolve(branchResults) } this.log.debug(`Adding branch protection ${JSON.stringify(params)}`) return this.github.rest.repos.updateBranchProtection(params).then(res => this.log.debug(`Branch protection applied successfully ${JSON.stringify(res.url)}`)).catch(e => { this.logError(`Error applying branch protection ${JSON.stringify(e)}`); return [] }) @@ -93,16 +93,16 @@ module.exports = class Branches extends ErrorStash { if (e.status === 404) { Object.assign(params, requiredBranchProtectionDefaults, Overrides.removeOverrides(overrides, branch.protection, {}), { headers: previewHeaders }) if (this.nop) { - resArray.push(new NopCommand(this.constructor.name, this.repo, this.github.rest.repos.updateBranchProtection.endpoint(params), 'Add Branch Protection')) - return Promise.resolve(resArray) + branchResults.push(new NopCommand(this.constructor.name, this.repo, this.github.rest.repos.updateBranchProtection.endpoint(params), 'Add Branch Protection')) + return Promise.resolve(branchResults) } this.log.debug(`Adding branch protection ${JSON.stringify(params)}`) return this.github.rest.repos.updateBranchProtection(params).then(res => this.log.debug(`Branch protection applied successfully ${JSON.stringify(res.url)}`)).catch(e => { this.logError(`Error applying branch protection ${JSON.stringify(e)}`); return [] }) } else { this.logError(e) if (this.nop) { - resArray.push(new NopCommand(this.constructor.name, this.repo, this.github.rest.repos.updateBranchProtection.endpoint(params), `${e}`, 'ERROR')) - return Promise.resolve(resArray) + branchResults.push(new NopCommand(this.constructor.name, this.repo, this.github.rest.repos.updateBranchProtection.endpoint(params), `${e}`, 'ERROR')) + return Promise.resolve(branchResults) } } }) diff --git a/lib/settings.js b/lib/settings.js index fd7ca2693..f918369c6 100644 --- a/lib/settings.js +++ b/lib/settings.js @@ -196,10 +196,14 @@ class Settings { return } - // remove duplicate rows in this.results + // Remove exact duplicate rows from this.results. Comparing only on + // type/repo/plugin would also drop legitimately distinct entries, e.g. the + // planned protection changes for two branches of the same repo. this.results = this.results.filter((thing, index, self) => { return index === self.findIndex((t) => { - return t.type === thing.type && t.repo === thing.repo && t.plugin === thing.plugin + return t.type === thing.type && t.repo === thing.repo && t.plugin === thing.plugin && + t.endpoint === thing.endpoint && JSON.stringify(t.body) === JSON.stringify(thing.body) && + JSON.stringify(t.action) === JSON.stringify(thing.action) }) }) diff --git a/test/unit/lib/plugins/branches.test.js b/test/unit/lib/plugins/branches.test.js index 49549cc87..c6994a9c7 100644 --- a/test/unit/lib/plugins/branches.test.js +++ b/test/unit/lib/plugins/branches.test.js @@ -363,6 +363,39 @@ describe('Branches', () => { }) }) + describe('in nop mode', () => { + function configureNop (config) { + return new Branches(true, github, { owner: 'bkeepers', repo: 'test' }, config, log, []) + } + + beforeEach(() => { + github.rest.repos.updateBranchProtection.endpoint = jest.fn().mockImplementation(params => { + return { url: 'updateBranchProtection', body: params } + }) + github.rest.repos.deleteBranchProtection.endpoint = jest.fn().mockImplementation(params => { + return { url: 'deleteBranchProtection', body: params } + }) + }) + + describe('when multiple branches have changes', () => { + it('reports each planned change exactly once', () => { + const plugin = configureNop( + [ + { name: 'master', protection: { enforce_admins: true } }, + { name: 'develop', protection: { enforce_admins: true } } + ] + ) + + return plugin.sync().then(res => { + const commands = res.filter(Boolean) + // Each branch produces two NopCommands: the diff record and the + // endpoint command. Two branches must yield four entries, not eight. + expect(commands.length).toBe(4) + }) + }) + }) + }) + describe.skip('return values', () => { it('returns updateBranchProtection Promise', () => { const plugin = configure( diff --git a/test/unit/lib/settings.test.js b/test/unit/lib/settings.test.js index e102379a6..b8959cadc 100644 --- a/test/unit/lib/settings.test.js +++ b/test/unit/lib/settings.test.js @@ -549,4 +549,38 @@ repository: expect(mockRepoSync).toHaveBeenCalledTimes(1) }) }) // updateRepos - archived repo skipping + + describe('handleResults', () => { + it('removes exact duplicates but keeps distinct entries for the same repo and plugin', async () => { + stubContext.octokit.rest.checks = { update: jest.fn().mockResolvedValue({}) } + stubContext.octokit.rest.issues = { createComment: jest.fn().mockResolvedValue({}) } + stubContext.payload.check_run = { id: 42, check_suite: { pull_requests: [{ number: 1 }] } } + stubContext.payload.repository = { owner: { login: 'test' }, name: 'test-repo' } + const settings = new Settings(true, stubContext, mockRepo, {}, mockRef) + + const masterDiff = { + type: 'INFO', + plugin: 'Branches', + repo: 'test/test-repo', + endpoint: '', + body: '', + action: { msg: 'Changes for master', additions: {}, modifications: { a: 1 }, deletions: {} } + } + const developDiff = { + type: 'INFO', + plugin: 'Branches', + repo: 'test/test-repo', + endpoint: '', + body: '', + action: { msg: 'Changes for develop', additions: {}, modifications: { b: 2 }, deletions: {} } + } + settings.results = [masterDiff, { ...masterDiff }, developDiff] + + await settings.handleResults() + + // the literal duplicate goes, the distinct same-plugin same-repo entry stays + expect(settings.results).toHaveLength(2) + expect(settings.results.map(res => res.action.msg)).toEqual(['Changes for master', 'Changes for develop']) + }) + }) // handleResults }) // Settings Tests From c70b320152577b9db539645729088e3cb00afc34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomas=20Daba=C5=A1inskas?= Date: Sat, 11 Jul 2026 14:11:20 +0300 Subject: [PATCH 2/2] fix(settings): optimize duplicate result filtering with Set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace O(n²) array filtering with O(n) Set-based deduplication for better performance when processing large result sets. The previous implementation used nested `findIndex` which became slow with many results. Also add null check to prevent errors when processing undefined results. --- lib/settings.js | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/lib/settings.js b/lib/settings.js index f918369c6..c5e017ba4 100644 --- a/lib/settings.js +++ b/lib/settings.js @@ -199,12 +199,17 @@ class Settings { // Remove exact duplicate rows from this.results. Comparing only on // type/repo/plugin would also drop legitimately distinct entries, e.g. the // planned protection changes for two branches of the same repo. - this.results = this.results.filter((thing, index, self) => { - return index === self.findIndex((t) => { - return t.type === thing.type && t.repo === thing.repo && t.plugin === thing.plugin && - t.endpoint === thing.endpoint && JSON.stringify(t.body) === JSON.stringify(thing.body) && - JSON.stringify(t.action) === JSON.stringify(thing.action) - }) + const seenResults = new Set() + this.results = this.results.filter(res => { + if (!res) { + return true + } + const key = JSON.stringify([res.type, res.repo, res.plugin, res.endpoint, res.body, res.action]) + if (seenResults.has(key)) { + return false + } + seenResults.add(key) + return true }) let error = false