From 23e7316279c453294fc06561e1c70ba843936256 Mon Sep 17 00:00:00 2001 From: Yadhav Jayaraman <57544838+decyjphr@users.noreply.github.com> Date: Fri, 7 Aug 2026 16:44:47 -0400 Subject: [PATCH] fix(rulesets): resolve {{EXTERNALLY_DEFINED}} before diffing (incorporate PR #1023) Diffable.sync() now calls an optional resolveOverrides(existingRecords, filteredEntries) hook after find() and before the comparison. The rulesets plugin implements it: each config entry is matched to its live record (via comparator) and passed through Overrides.removeOverrides, so {{EXTERNALLY_DEFINED}} placeholders resolve to the live values before changed() ever sees them. This mirrors how branches.js resolves overrides inside compareDeep. Behavior change (intentional, fixes #1022): a ruleset whose only difference from GitHub is the placeholder no longer reports "Update Ruleset" in dry runs and no longer issues a redundant PUT in apply mode. Rulesets with real differences behave exactly as before. Entries are cloned (structuredClone) before resolution since removeOverrides mutates its input. Tests updated to the new behavior and nop-mode coverage added. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- lib/plugins/diffable.js | 5 ++ lib/plugins/rulesets.js | 11 ++++ test/unit/lib/plugins/rulesets.test.js | 78 ++++++++++++++++---------- 3 files changed, 64 insertions(+), 30 deletions(-) diff --git a/lib/plugins/diffable.js b/lib/plugins/diffable.js index 57d4cae17..1b5774160 100644 --- a/lib/plugins/diffable.js +++ b/lib/plugins/diffable.js @@ -74,6 +74,11 @@ module.exports = class Diffable extends ErrorStash { let filteredEntries = this.filterEntries() // this.log.debug(`filtered entries are ${JSON.stringify(filteredEntries)}`) return this.find().then(existingRecords => { + // Let plugins resolve config placeholders (e.g. {{EXTERNALLY_DEFINED}}) against + // the live records before any comparison, so placeholders never report changes. + if (typeof this.resolveOverrides === 'function') { + filteredEntries = this.resolveOverrides(existingRecords, filteredEntries) + } this.log.debug(` ${JSON.stringify(existingRecords, null, 2)} \n\n ${JSON.stringify(filteredEntries, null, 2)} `) const mergeDeep = new MergeDeep(this.log, this.github, ignorableFields) diff --git a/lib/plugins/rulesets.js b/lib/plugins/rulesets.js index 24daaf121..36792a2be 100644 --- a/lib/plugins/rulesets.js +++ b/lib/plugins/rulesets.js @@ -239,6 +239,17 @@ module.exports = class Rulesets extends Diffable { return existing.name === attrs.name } + // Resolve {{EXTERNALLY_DEFINED}} placeholders against the matching live ruleset + // before diffing, mirroring how branches.js resolves overrides inside its + // comparison. Without this the comparison sees the literal placeholder string, + // so every plan reports an update for rulesets that use overrides. + resolveOverrides (existingRecords, entries) { + return entries.map(attrs => { + const existing = existingRecords.find(record => this.comparator(record, attrs)) + return Overrides.removeOverrides(overrides, structuredClone(attrs), existing || {}) + }) + } + changed (existing, attrs) { const mergeDeep = new MergeDeep(this.log, this.github, ignorableFields) const merged = mergeDeep.compareDeep(existing, attrs) diff --git a/test/unit/lib/plugins/rulesets.test.js b/test/unit/lib/plugins/rulesets.test.js index c3cc475d4..609750e42 100644 --- a/test/unit/lib/plugins/rulesets.test.js +++ b/test/unit/lib/plugins/rulesets.test.js @@ -292,7 +292,7 @@ describe('Rulesets', () => { }) describe('when {{EXTERNALLY_DEFINED}} is present in "required_status_checks" and status checks exist in GitHub', () => { - it('it retains the status checks from GitHub and everything else is reset to the safe-settings', () => { + it('skips the placeholder-only ruleset and updates the genuinely changed ones', () => { // Mock the GitHub API response github.paginate = jest.fn().mockResolvedValue([ generateRequestRuleset( @@ -355,22 +355,12 @@ describe('Rulesets', () => { ) return plugin.sync().then(() => { + // Ruleset 1 only differs by the {{EXTERNALLY_DEFINED}} placeholder, which + // resolves to the live status checks, so it must not be updated at all. + expect(github.request).toHaveBeenCalledTimes(2) expect(github.request).toHaveBeenNthCalledWith( 1, 'PUT /repos/{owner}/{repo}/rulesets/{id}', - generateResponseRuleset( - 1, - 'All branches 1', - repo_conditions, - [ - { context: 'Custom Check 1' }, - { context: 'Custom Check 2' } - ] - ) - ) - expect(github.request).toHaveBeenNthCalledWith( - 2, - 'PUT /repos/{owner}/{repo}/rulesets/{id}', generateResponseRuleset( 2, 'All branches 2', @@ -382,7 +372,7 @@ describe('Rulesets', () => { ) ) expect(github.request).toHaveBeenNthCalledWith( - 3, + 2, 'PUT /repos/{owner}/{repo}/rulesets/{id}', generateResponseRuleset( 3, @@ -473,7 +463,7 @@ describe('Rulesets', () => { }) describe('[org] when {{EXTERNALLY_DEFINED}} is present in "required_status_checks" and status checks exist in GitHub', () => { - it('it retains the status checks from GitHub', () => { + it('reports no changes when only the placeholder differs from GitHub', () => { // Mock the GitHub API response github.paginate = jest.fn().mockResolvedValue([ generateRequestRuleset( @@ -506,20 +496,48 @@ describe('Rulesets', () => { ) return plugin.sync().then(() => { - expect(github.request).toHaveBeenNthCalledWith( - 1, - 'PUT /orgs/{org}/rulesets/{id}', - generateResponseRuleset( - 1, - 'All branches 1', - org_conditions, - [ - { context: 'Custom Check 1' }, - { context: 'Custom Check 2' } - ], - true - ) - ) + // The placeholder resolves to the live status checks, so the ruleset is unchanged + expect(github.request).not.toHaveBeenCalled() + }) + }) + }) + + describe('in nop mode', () => { + beforeEach(() => { + github.request.endpoint = Object.assign( + jest.fn().mockImplementation((route, parms) => { return { url: route, body: parms } }), + { merge: github.request.endpoint.merge } + ) + }) + + it('does not plan an update when {{EXTERNALLY_DEFINED}} matches the status checks in GitHub', () => { + github.paginate = jest.fn().mockResolvedValue([ + generateRequestRuleset(1, 'All branches 1', repo_conditions, [{ context: 'Custom Check 1' }]) + ]) + + const plugin = configure([ + generateRequestRuleset(1, 'All branches 1', repo_conditions, [{ context: '{{EXTERNALLY_DEFINED}}' }]) + ], 'repo', true) + + return plugin.sync().then(res => { + // sync resolves with nothing when no changes are detected + const messages = (res || []).flat(2).map(nopCommand => nopCommand.action?.msg) + expect(messages).not.toContain('Update Ruleset') + }) + }) + + it('still plans an update when the config genuinely differs from GitHub', () => { + github.paginate = jest.fn().mockResolvedValue([ + generateRequestRuleset(1, 'All branches 1', repo_conditions, [{ context: 'Custom Check 1' }]) + ]) + + const plugin = configure([ + generateRequestRuleset(1, 'All branches 1', repo_conditions, [{ context: 'Other Check' }]) + ], 'repo', true) + + return plugin.sync().then(res => { + const messages = res.flat(2).map(nopCommand => nopCommand.action?.msg) + expect(messages).toContain('Update Ruleset') }) }) })