From 0452444f2ed014247953f7e9c5083a402ac66c63 Mon Sep 17 00:00:00 2001 From: Yadhav Jayaraman <57544838+decyjphr@users.noreply.github.com> Date: Wed, 5 Aug 2026 19:13:47 -0400 Subject: [PATCH 1/3] fix(settings): handle full-sync NOP results without check run Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- lib/settings.js | 12 ++++++++++++ test/unit/lib/settings.test.js | 29 +++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/lib/settings.js b/lib/settings.js index 02bb6526..d334ef6e 100644 --- a/lib/settings.js +++ b/lib/settings.js @@ -1161,6 +1161,18 @@ class Settings { }) } + // Full-sync NOP runs do not have the webhook fields needed to report to a + // check run. Keep potentially sensitive diff values at debug level and log + // only a value-free summary at info level. + if (!payload?.check_run || !payload?.repository) { + this.log.debug({ results: this.results }, 'Dry-run results') + const summary = this.results + .map(res => `${res.type} ${res.plugin} ${res.repo}: ${res.action?.msg ?? ''}`) + .join('\n') + this.log.info(`Dry-run finished with ${this.results.length} planned change(s); the full diff is logged at debug level.\n${summary}`) + return + } + let error = false const stats = { reposProcessed: {}, diff --git a/test/unit/lib/settings.test.js b/test/unit/lib/settings.test.js index 93de139f..133ae753 100644 --- a/test/unit/lib/settings.test.js +++ b/test/unit/lib/settings.test.js @@ -1057,6 +1057,35 @@ repository: expect(msgs.some(m => /teams/.test(m))).toBe(true) }) + it.each([ + ['without a check run', {}], + ['without a repository', { check_run: { id: 123 } }] + ])('28. full-sync dry run %s logs a value-free summary instead of updating a check run', async (_description, payload) => { + stubContext.payload = { installation: { id: 123 }, ...payload } + stubContext.octokit.checks = { update: jest.fn().mockResolvedValue({}) } + + const settings = new Settings(true, stubContext, mockRepo, {}, mockRef) + settings.results = [{ + type: 'INFO', + plugin: 'Variables', + repo: 'test/test-repo', + endpoint: '', + action: { + msg: 'Changes found', + additions: {}, + modifications: { MY_VAR: { value: 'plain-value' } }, + deletions: {} + } + }] + + await settings.handleResults() + + expect(stubContext.log.info).toHaveBeenCalledWith(expect.stringContaining('Changes found')) + expect(stubContext.log.info).not.toHaveBeenCalledWith(expect.stringContaining('plain-value')) + expect(stubContext.log.debug).toHaveBeenCalledWith({ results: settings.results }, 'Dry-run results') + expect(stubContext.octokit.checks.update).not.toHaveBeenCalled() + }) + it('28. base-config filtering preserves org-rulesets informational NopCommands', async () => { stubContext.payload.repository = { owner: { login: 'test' }, name: 'safe-settings' } stubContext.payload.check_run = { id: 123, check_suite: { pull_requests: [{ number: 456 }] } } From 4234c7add2278bbf62e789647b79141f7ae1512d Mon Sep 17 00:00:00 2001 From: Yadhav Jayaraman <57544838+decyjphr@users.noreply.github.com> Date: Thu, 6 Aug 2026 10:23:36 -0400 Subject: [PATCH 2/3] fix(rulesets): make ruleset create idempotent on duplicate name A ruleset POST is not idempotent. When Octokit retries a create that already succeeded, or a repo is processed by two overlapping syncs (full sync racing with repository.created), the second create fails with 422 'Name must be unique'. Reconcile by looking the existing ruleset up by name and updating it in place. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- lib/plugins/rulesets.js | 37 +++++++++++- test/unit/lib/plugins/rulesets.test.js | 78 ++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 2 deletions(-) diff --git a/lib/plugins/rulesets.js b/lib/plugins/rulesets.js index e0238364..09977840 100644 --- a/lib/plugins/rulesets.js +++ b/lib/plugins/rulesets.js @@ -291,7 +291,7 @@ module.exports = class Rulesets extends Diffable { this.log.debug(`Ruleset created successfully ${JSON.stringify(res.url)}`) return res }).catch(e => { - return this.handleError(e) + return this.handleDuplicateOrError(e, attrs) }) } else { if (this.nop) { @@ -305,11 +305,44 @@ module.exports = class Rulesets extends Diffable { this.log.debug(`Ruleset created successfully ${JSON.stringify(res.url)}`) return res }).catch(e => { - return this.handleError(e) + return this.handleDuplicateOrError(e, attrs) }) } } + // A ruleset create (POST) is not idempotent. Octokit's retry / auth-app + // layers can re-send a POST that already succeeded, and a repo can also be + // processed by two overlapping syncs (e.g. a full sync racing with the + // repository.created webhook). In both cases the second create hits + // "Name must be unique" (422) even though the ruleset now exists. Instead of + // failing the whole run, reconcile by looking the ruleset up by name and + // updating it in place so the create effectively becomes idempotent. + isDuplicateNameError (e) { + if (!e || e.status !== 422) return false + const errors = e.response && e.response.data && e.response.data.errors + const list = Array.isArray(errors) ? errors : [] + return list.some(err => { + const msg = typeof err === 'string' ? err : (err && err.message) + return typeof msg === 'string' && /name must be unique/i.test(msg) + }) + } + + handleDuplicateOrError (e, attrs) { + if (!this.isDuplicateNameError(e)) { + return this.handleError(e) + } + this.log.debug(`Ruleset '${attrs && attrs.name}' already exists (concurrent or retried create); reconciling by update`) + return this.find().then(existing => { + const match = Array.isArray(existing) + ? existing.find(record => this.comparator(record, attrs)) + : undefined + if (!match) { + return this.handleError(e) + } + return this.update(match, attrs) + }).catch(err => this.handleError(err)) + } + remove (existing) { const parms = this.wrapAttrs(Object.assign({ id: existing.id })) if (this.scope === 'org') { diff --git a/test/unit/lib/plugins/rulesets.test.js b/test/unit/lib/plugins/rulesets.test.js index df72e0ce..c3cc475d 100644 --- a/test/unit/lib/plugins/rulesets.test.js +++ b/test/unit/lib/plugins/rulesets.test.js @@ -179,6 +179,84 @@ describe('Rulesets', () => { }) }) + describe('idempotent create when the ruleset already exists (retried/concurrent POST)', () => { + function duplicateNameError () { + const e = new Error('Validation Failed') + e.status = 422 + e.response = { data: { errors: ['Name must be unique'] } } + return e + } + + function wireRequest (routeResults) { + const calls = [] + const request = jest.fn().mockImplementation((route, body) => { + calls.push({ route, body }) + const handler = routeResults[route] + return handler ? handler() : Promise.resolve({ url: route }) + }) + request.endpoint = jest.fn().mockImplementation((route, body) => ({ url: route, body })) + request.endpoint.merge = jest.fn().mockImplementation((route, body) => ({ method: 'GET', url: route, ...body })) + github.request = request + return calls + } + + it('reconciles a repo ruleset by updating the existing one on 422 "Name must be unique"', async () => { + const attrs = generateRequestRuleset(0, 'synk', repo_conditions, []) + delete attrs.id + const existing = generateResponseRuleset(42, 'synk', repo_conditions, []) + const calls = wireRequest({ + 'POST /repos/{owner}/{repo}/rulesets': () => Promise.reject(duplicateNameError()) + }) + github.paginate = jest.fn() + .mockResolvedValueOnce([{ id: 42, name: 'synk', source_type: 'Repository' }]) + .mockResolvedValueOnce([existing]) + + const plugin = configure([attrs]) + await plugin.add(attrs) + + const put = calls.find(c => c.route === 'PUT /repos/{owner}/{repo}/rulesets/{id}') + expect(put).toBeDefined() + expect(put.body.id).toBe(42) + }) + + it('reconciles an org ruleset by updating the existing one on 422 "Name must be unique"', async () => { + const attrs = generateRequestRuleset(0, 'synk', org_conditions, [], true) + delete attrs.id + const existing = generateResponseRuleset(7, 'synk', org_conditions, [], true) + const calls = wireRequest({ + 'POST /orgs/{org}/rulesets': () => Promise.reject(duplicateNameError()) + }) + github.paginate = jest.fn() + .mockResolvedValueOnce([{ id: 7, name: 'synk', source_type: 'Organization' }]) + .mockResolvedValueOnce([existing]) + + const plugin = configure([attrs], 'org') + await plugin.add(attrs) + + const put = calls.find(c => c.route === 'PUT /orgs/{org}/rulesets/{id}') + expect(put).toBeDefined() + expect(put.body.id).toBe(7) + }) + + it('does not reconcile (surfaces the error) for a 422 that is not a name-uniqueness violation', async () => { + const attrs = generateRequestRuleset(0, 'synk', repo_conditions, []) + delete attrs.id + const other = new Error('Validation Failed') + other.status = 422 + other.response = { data: { errors: ['Something else is invalid'] } } + const calls = wireRequest({ + 'POST /repos/{owner}/{repo}/rulesets': () => Promise.reject(other) + }) + github.paginate = jest.fn() + + const plugin = configure([attrs]) + await plugin.add(attrs) + + expect(github.paginate).not.toHaveBeenCalled() + expect(calls.some(c => c.route === 'PUT /repos/{owner}/{repo}/rulesets/{id}')).toBe(false) + }) + }) + describe('when {{EXTERNALLY_DEFINED}} is present in "required_status_checks" and no status checks exist in GitHub', () => { it('it initialises the status checks with an empty list', () => { // Mock the GitHub API response From 00b54a261de9d2bdd3b3ff9e024d4b4f45b93f68 Mon Sep 17 00:00:00 2001 From: Yadhav Jayaraman <57544838+decyjphr@users.noreply.github.com> Date: Thu, 6 Aug 2026 22:24:12 -0400 Subject: [PATCH 3/3] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- lib/plugins/rulesets.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/plugins/rulesets.js b/lib/plugins/rulesets.js index 09977840..24daaf12 100644 --- a/lib/plugins/rulesets.js +++ b/lib/plugins/rulesets.js @@ -339,7 +339,8 @@ module.exports = class Rulesets extends Diffable { if (!match) { return this.handleError(e) } - return this.update(match, attrs) + const { id: _ignoredId, ...attrsWithoutId } = attrs || {} + return this.update(match, attrsWithoutId) }).catch(err => this.handleError(err)) }