From e4b5f0060da1b3ef1bdab42e4d1e62d6caf73169 Mon Sep 17 00:00:00 2001 From: Yadhav Jayaraman <57544838+decyjphr@users.noreply.github.com> Date: Mon, 28 Sep 2026 14:43:07 -0400 Subject: [PATCH 1/3] fix(mergedeep): converge identifier-less bypass actors Adapt PR #1035 to the existing comparison logic: ignore IDs only for OrganizationAdmin and DeployKey, preserve meaningful actor changes, and add unit, consumer, and focused smoke coverage. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- README.md | 4 ++ docs/sample-settings/settings.yml | 5 +- lib/mergeDeep.js | 59 ++++++++--------- smoke-test.js | 87 +++++++++++++++++++++++- test/unit/lib/mergeDeep.test.js | 91 ++++++++++++++++++++++++++ test/unit/lib/plugins/rulesets.test.js | 64 ++++++++++++++++++ 6 files changed, 278 insertions(+), 32 deletions(-) diff --git a/README.md b/README.md index c9147bd07..13e00039c 100644 --- a/README.md +++ b/README.md @@ -972,6 +972,9 @@ node smoke-test.js --phase 1-3 npm run smoke-test:phase -- 1,3,5 node smoke-test.js --phase 1,3,5 +# Bypass actor apply/NOP convergence (Phase 1 creates the required test repo) +node smoke-test.js --phase 1,19 + # Mix range + interactive npm run smoke-test:phase -- 1-3 interactive node smoke-test.js --phase 1-3 --interactive @@ -997,6 +1000,7 @@ The smoke test runs the following phases: | **Phase 11** | Validates `additive_plugins` — verifies additive-mode plugin behaviour | | **Phase 12** | Tests `custom_properties` plugin | | **Phase 13** | Tests the `variables` plugin (create, update, remove variables) | +| **Phase 19** | Tests ignored `OrganizationAdmin`/`DeployKey` IDs, order-independent NOP convergence, real bypass-mode/role-ID changes, and no redundant updates (requires Phase 1) | | **Teardown** | Shuts down safe-settings, deletes test repos, teams, custom roles, and rulesets | ### Output diff --git a/docs/sample-settings/settings.yml b/docs/sample-settings/settings.yml index edc2b2dcb..64604b37d 100644 --- a/docs/sample-settings/settings.yml +++ b/docs/sample-settings/settings.yml @@ -253,6 +253,7 @@ rulesets: # - Team # - Integration # - OrganizationAdmin + # - DeployKey actor_type: Team # When the specified actor can bypass the ruleset. `pull_request` # means that an actor can only bypass rules on pull requests. @@ -260,7 +261,9 @@ rulesets: # - pull_request bypass_mode: pull_request - - actor_id: 1 + # GitHub ignores actor_id for OrganizationAdmin and DeployKey and returns + # null. Use null (or omit actor_id) for these actor types. + - actor_id: null actor_type: OrganizationAdmin bypass_mode: always diff --git a/lib/mergeDeep.js b/lib/mergeDeep.js index 9cb82437d..600462ede 100644 --- a/lib/mergeDeep.js +++ b/lib/mergeDeep.js @@ -3,7 +3,8 @@ const DeploymentConfig = require('./deploymentConfig') const NAME_FIELDS = ['name', 'username', 'actor_id', 'login', 'type', 'key_prefix', 'context'] const NAME_USERNAME_PROPERTY = item => NAME_FIELDS.find(prop => Object.prototype.hasOwnProperty.call(item, prop)) -const GET_NAME_USERNAME_PROPERTY = item => { if (NAME_USERNAME_PROPERTY(item)) return item[NAME_USERNAME_PROPERTY(item)] } +const ignoresActorId = item => (item.actor_type === 'OrganizationAdmin' || item.actor_type === 'DeployKey') && + Object.prototype.hasOwnProperty.call(item, 'bypass_mode') // Fields within a rule's `parameters` that are managed/defaulted by the GitHub API. // They should not be treated as user-driven deletions when omitted from config. @@ -24,23 +25,24 @@ const stableStringify = value => { return JSON.stringify(value) } -// Compute the identity value for an array element so the same logical item in -// `source` (config) and `target` (GitHub API) can be paired during comparison. -// Returns the raw identifying value (so a bare string shorthand like 'developers' -// still matches an object like { name: 'developers' }). Special-cases bypass -// actors: GitHub returns `actor_id: null` for role-based actor types such as -// `OrganizationAdmin`, so we key those on `actor_type` to avoid spurious -// add/delete churn when config supplies an explicit id. -const getItemIdentity = item => { +// Use the same identifying field for array pairing and displayed modifications. +// GitHub ignores IDs for OrganizationAdmin and DeployKey, so key them by type. +const getItemIdentityProperty = item => { if (!item || typeof item !== 'object' || Array.isArray(item)) return undefined if (Object.prototype.hasOwnProperty.call(item, 'actor_type') && Object.prototype.hasOwnProperty.call(item, 'bypass_mode')) { - if (item.actor_id === null || item.actor_id === undefined || item.actor_type === 'OrganizationAdmin') { - return item.actor_type + if (item.actor_id === null || item.actor_id === undefined || ignoresActorId(item)) { + return 'actor_type' } - return item.actor_id + return 'actor_id' } - return GET_NAME_USERNAME_PROPERTY(item) + return NAME_USERNAME_PROPERTY(item) +} + +const getItemIdentity = item => { + const property = getItemIdentityProperty(item) + // Keep raw values so string shorthands still match objects with a name. + return property === undefined ? undefined : item[property] } class MergeDeep { @@ -140,6 +142,10 @@ class MergeDeep { if (key.indexOf('url') >= 0 || this.ignorableFields.indexOf(key) >= 0) { continue } + // These actor types have no meaningful ID, even when config supplies one. + if (key === 'actor_id' && ignoresActorId(source) && target.actor_type === source.actor_type) { + continue + } const sourceValue = source[key] const targetValue = target[key] if (targetValue === undefined) { @@ -171,20 +177,13 @@ class MergeDeep { } } else { // The entry is a simple primitive if (targetValue !== sourceValue) { - // GitHub returns `actor_id: null` for role-based bypass actor types - // (e.g. OrganizationAdmin) regardless of the id supplied in config. - // Don't treat that placeholder mismatch as a modification. - if (key === 'actor_id' && (targetValue === null || sourceValue === null)) { - // treat as equal - } else { - // Note: source[key] cannot be undefined here since we are iterating on source keys - // so we don't need to check for that. - // The entries are different. It is an addition - modifications[key] = sourceValue - // retroactively add `name` or `username` to the modifications - // Since those are the only fields that can be used to identify the resource - this.addIdentifyingAttribute(source, key, modifications) - } + // Note: source[key] cannot be undefined here since we are iterating on source keys + // so we don't need to check for that. + // The entries are different. It is an addition + modifications[key] = sourceValue + // retroactively add `name` or `username` to the modifications + // Since those are the only fields that can be used to identify the resource + this.addIdentifyingAttribute(source, key, modifications) } else { // The entry is the same in both objects } @@ -220,10 +219,10 @@ class MergeDeep { } addIdentifyingAttribute (source, key, containerObject) { - const id = NAME_USERNAME_PROPERTY(source) + const id = getItemIdentityProperty(source) if (id) { this.log.debug(`Adding name for ${key} ${source[key]}`) - containerObject[id] = GET_NAME_USERNAME_PROPERTY(source) + containerObject[id] = source[id] } } @@ -351,7 +350,7 @@ class MergeDeep { // Add name attribute to the modifications to make it look better ; it won't be added otherwise as it would be the same if (!this.isEmpty(modifications[modifications.length - 1])) { if (visited[visitedId]) { - const displayProp = NAME_USERNAME_PROPERTY(a) + const displayProp = getItemIdentityProperty(a) if (displayProp) { modifications[modifications.length - 1][displayProp] = a[displayProp] } diff --git a/smoke-test.js b/smoke-test.js index 30ef1a643..4eee74620 100644 --- a/smoke-test.js +++ b/smoke-test.js @@ -3185,6 +3185,90 @@ async function phase18TeamIncludeExclude () { await deleteBranch(ORG, ADMIN_REPO, branch) } +async function phase19BypassActorConvergence () { + logPhase('Phase 19: Ruleset ignored bypass actor IDs and convergence') + const defaultBranch = await getDefaultBranch() + const name = 'smoke-null-bypass-actors' + const actor = (actorType, actorId, bypassMode = 'always') => ({ + actor_type: actorType, + ...(actorId === undefined ? {} : { actor_id: actorId }), + bypass_mode: bypassMode + }) + const steps = [ + ['19a', 'create', [ + actor('OrganizationAdmin', 1), actor('DeployKey', 1), actor('RepositoryRole', 4), actor('RepositoryRole', 5) + ], true], + ['19b', 'reorder and omit ignored ids', [ + actor('RepositoryRole', 5), actor('DeployKey', null), actor('OrganizationAdmin', undefined), actor('RepositoryRole', 4) + ], false], + ['19c', 'change bypass mode and a real role id', [ + actor('OrganizationAdmin', null, 'pull_request'), actor('DeployKey', null), actor('RepositoryRole', 3), actor('RepositoryRole', 5) + ], true], + ['19d', 'converge with concrete ignored ids', [ + actor('RepositoryRole', 5), actor('DeployKey', 9), actor('RepositoryRole', 3), actor('OrganizationAdmin', 8, 'pull_request') + ], false] + ] + const sortActors = actors => actors.slice().sort((a, b) => + `${a.actor_type}:${a.actor_id}`.localeCompare(`${b.actor_type}:${b.actor_id}`)) + let previous + + for (const [id, description, actors, changes] of steps) { + const branch = `smoke-test-phase${id}` + const attrs = { + name, + target: 'branch', + enforcement: 'disabled', + conditions: { ref_name: { include: ['~DEFAULT_BRANCH'], exclude: [] } }, + bypass_actors: actors, + rules: [{ type: 'deletion' }] + } + const rulesetYaml = require('js-yaml').dump([attrs]) + await deleteBranch(ORG, ADMIN_REPO, branch) + await createBranch(ORG, ADMIN_REPO, branch) + await createOrUpdateFile(ORG, ADMIN_REPO, `${CONFIG_PATH}/repos/test.yml`, REPO_TEST_YML + rulesetYaml, branch, `${id}: ${description}`) + const pr = await createPR(ORG, ADMIN_REPO, `${id}: bypass actors ${description}`, branch, defaultBranch) + + await sleep(WEBHOOK_SETTLE_MS) + const checkRun = await waitForCheckRun(ORG, ADMIN_REPO, pr.head.sha) + if (!assert(checkRun && checkRun.conclusion === 'success', `${id}: NOP check completed successfully`)) { + throw new Error(`${id}: cannot apply without a successful NOP check`) + } + const summary = checkRun.output && checkRun.output.summary + if (changes) { + assert(summary && summary.includes(name), `${id}: NOP reports the changed bypass actor policy`) + } else { + assert(summary && /No changes to apply/i.test(summary), `${id}: NOP reports no changes after ignored-id/reordering changes`) + } + + if (!await safeMerge(ORG, ADMIN_REPO, pr.number)) return + await sleep(WEBHOOK_SETTLE_MS) + const expectedActors = sortActors(actors.map(entry => actor( + entry.actor_type, + entry.actor_type === 'OrganizationAdmin' || entry.actor_type === 'DeployKey' ? null : entry.actor_id, + entry.bypass_mode + ))) + const details = await poll(async () => { + const ruleset = await getRepoRuleset(ORG, 'test', name) + if (!ruleset) return null + const live = await getRepoRulesetDetails(ORG, 'test', ruleset.id) + if (!live || !Array.isArray(live.bypass_actors)) return null + const actualActors = sortActors(live.bypass_actors.map(entry => actor(entry.actor_type, entry.actor_id, entry.bypass_mode))) + return JSON.stringify(actualActors) === JSON.stringify(expectedActors) ? live : null + }, { desc: `${id}: exact bypass actors, modes and real role ids to be applied` }) + if (!assert(details !== null, `${id}: exact actor set applied with null ignored IDs and distinct real IDs`)) { + throw new Error(`${id}: bypass actor state did not converge`) + } + if (previous) { + assert(details.id === previous.id, `${id}: existing ruleset updated in place`) + if (!changes) { + assert(typeof details.updated_at === 'string' && details.updated_at === previous.updated_at, `${id}: second sync did not rewrite the ruleset`) + } + } + previous = details + await deleteBranch(ORG, ADMIN_REPO, branch) + } +} + async function main () { const { App } = await import('octokit') const app = new App({ appId: APP_ID, privateKey: PRIVATE_KEY }) @@ -3253,7 +3337,8 @@ async function main () { ['Phase 15: Ruleset array drift', phase15RulesetArrayDrift], ['Phase 16: Ruleset name/slug resolution', phase16RulesetNameResolution], ['Phase 17: App installation management', phase17AppInstallations], - ['Phase 18: Team include/exclude filters', phase18TeamIncludeExclude] + ['Phase 18: Team include/exclude filters', phase18TeamIncludeExclude], + ['Phase 19: Bypass actor convergence', phase19BypassActorConvergence] ] // When --phase is given, only run setup (phase 0) + the requested phase(s). diff --git a/test/unit/lib/mergeDeep.test.js b/test/unit/lib/mergeDeep.test.js index f3446ce0f..2a60ceb4d 100644 --- a/test/unit/lib/mergeDeep.test.js +++ b/test/unit/lib/mergeDeep.test.js @@ -4,6 +4,97 @@ const YAML = require('js-yaml') const log = require('pino')('test.log') describe('MergeDeep Test', () => { + describe('bypass actor comparison', () => { + const mergeDeep = new MergeDeep(log, jest.fn()) + const actor = (actorType, actorId, bypassMode = 'always') => ({ + actor_type: actorType, + ...(actorId === undefined ? {} : { actor_id: actorId }), + bypass_mode: bypassMode + }) + const admin = actor('OrganizationAdmin', null) + const deployKey = actor('DeployKey', null) + const app = actor('Integration', 210920) + + it.each([ + ['empty ruleset', [], [admin], [admin]], + ['existing app', [app], [app, admin], [admin]], + ['distinct null-id actor', [admin], [admin, deployKey], [deployKey]] + ])('adds a null-id actor to %s', (_label, target, source, additions) => { + expect(mergeDeep.compareDeep({ bypass_actors: target }, { bypass_actors: source })).toStrictEqual({ + additions: { bypass_actors: additions }, modifications: {}, deletions: {}, hasChanges: true + }) + }) + + it.each([ + [[admin], [admin]], + [[app, admin], [app, admin]], + [[admin, deployKey, app], [app, deployKey, admin]] + ])('converges with distinct null-id and real-id actors %p', (target, source) => { + expect(mergeDeep.compareDeep({ bypass_actors: target }, { bypass_actors: source })).toStrictEqual({ + additions: {}, modifications: {}, deletions: {}, hasChanges: false + }) + }) + + describe.each(['OrganizationAdmin', 'DeployKey'])('%s ignores actor_id', actorType => { + const ids = [undefined, null, 1, 2] + it.each(ids.flatMap(live => ids.map(config => [live, config])))('converges for live %p and configured %p without mutation', (liveId, configId) => { + const target = Object.freeze({ bypass_actors: Object.freeze([Object.freeze(actor(actorType, liveId))]) }) + const source = Object.freeze({ bypass_actors: Object.freeze([Object.freeze(actor(actorType, configId))]) }) + const before = structuredClone({ target, source }) + + expect(mergeDeep.compareDeep(target, source)).toStrictEqual({ + additions: {}, modifications: {}, deletions: {}, hasChanges: false + }) + expect({ target, source }).toEqual(before) + }) + + it('reports real bypass_mode changes with the actor type as identity', () => { + expect(mergeDeep.compareDeep( + { bypass_actors: [actor(actorType, null)] }, + { bypass_actors: [actor(actorType, 1, 'pull_request')] } + )).toStrictEqual({ + additions: {}, + modifications: { bypass_actors: [{ actor_type: actorType, bypass_mode: 'pull_request' }] }, + deletions: {}, + hasChanges: true + }) + }) + }) + + it('deletes only the removed null-id actor', () => { + expect(mergeDeep.compareDeep( + { bypass_actors: [deployKey, admin, app] }, + { bypass_actors: [app, actor('OrganizationAdmin', 1)] } + )).toStrictEqual({ + additions: {}, modifications: {}, deletions: { bypass_actors: [deployKey] }, hasChanges: true + }) + }) + + it.each(['Team', 'Integration', 'RepositoryRole', 'User'])('preserves real %s id changes and distinct actors of the same type', actorType => { + const unchanged = actor(actorType, 7) + const removed = actor(actorType, 42) + const added = actor(actorType, 99) + expect(mergeDeep.compareDeep( + { bypass_actors: [removed, unchanged, admin] }, + { bypass_actors: [actor('OrganizationAdmin', 1), unchanged, added] } + )).toStrictEqual({ + additions: { bypass_actors: [added] }, modifications: {}, deletions: { bypass_actors: [removed] }, hasChanges: true + }) + expect(mergeDeep.compareDeep(actor(actorType, null), removed)).toStrictEqual({ + additions: {}, modifications: { actor_id: 42 }, deletions: {}, hasChanges: true + }) + }) + + it('preserves name precedence for non-bypass objects', () => { + expect(mergeDeep.compareDeep( + [{ name: 'policy', actor_id: 1, type: 'old' }], + [{ name: 'policy', actor_id: 2, type: 'new' }] + )).toStrictEqual({ + additions: [], modifications: [{ name: 'policy', actor_id: 2, type: 'new' }], deletions: [], hasChanges: true + }) + }) + }) + describe('compareDeep result shape', () => { const existing = { name: 'existing', enforcement: 'active' } const added = { name: 'new', enforcement: 'active' } diff --git a/test/unit/lib/plugins/rulesets.test.js b/test/unit/lib/plugins/rulesets.test.js index ccc92489f..0477a9dfa 100644 --- a/test/unit/lib/plugins/rulesets.test.js +++ b/test/unit/lib/plugins/rulesets.test.js @@ -188,6 +188,70 @@ describe('Rulesets', () => { }) }) + describe.each(['repo', 'org'])('identifier-less bypass actors (%s scope)', scope => { + const actor = (actorType, actorId, bypassMode = 'always') => ({ actor_type: actorType, actor_id: actorId, bypass_mode: bypassMode }) + const policy = bypassActors => ({ + name: 'Bypass policy', + target: 'branch', + enforcement: 'active', + conditions: { ref_name: { include: ['~ALL'], exclude: [] } }, + bypass_actors: bypassActors, + rules: [] + }) + + it.each([false, true])('converges without mutating config or scheduling an update (nop=%p)', async nop => { + const attrs = policy([actor('DeployKey', 123), actor('OrganizationAdmin', 1), actor('Integration', 42)]) + const before = structuredClone(attrs) + const existing = { ...policy([actor('Integration', 42), actor('OrganizationAdmin', null), actor('DeployKey', null)]), id: 7 } + const plugin = configure([attrs], scope, nop) + plugin.find = jest.fn().mockResolvedValue([existing]) + + expect(plugin.changed(existing, attrs)).toBe(false) + expect(await plugin.sync()).toBeUndefined() + expect(plugin.hasChanges).toBe(false) + expect(github.request).not.toHaveBeenCalled() + expect(attrs).toEqual(before) + }) + + it.each([false, true])('updates a real bypass_mode change and then converges (nop=%p)', async nop => { + const attrs = policy([actor('DeployKey', 123, 'pull_request'), actor('OrganizationAdmin', 1)]) + const existing = { ...policy([actor('OrganizationAdmin', null), actor('DeployKey', null)]), id: 7 } + const plugin = configure([attrs], scope, nop) + plugin.find = jest.fn().mockResolvedValue([existing]) + + expect(plugin.changed(existing, attrs)).toBe(true) + const result = await plugin.sync() + expect(plugin.hasChanges).toBe(true) + const route = scope === 'org' ? 'PUT /orgs/{org}/rulesets/{id}' : 'PUT /repos/{owner}/{repo}/rulesets/{id}' + if (nop) { + expect(github.request).not.toHaveBeenCalled() + expect(result.flat().filter(command => command.action.msg === 'Update Ruleset')).toHaveLength(1) + expect(github.request.endpoint).toHaveBeenCalledWith(route, expect.objectContaining({ id: 7, bypass_actors: attrs.bypass_actors })) + } else { + expect(github.request).toHaveBeenCalledTimes(1) + expect(github.request).toHaveBeenCalledWith(route, expect.objectContaining({ id: 7, bypass_actors: attrs.bypass_actors })) + } + + plugin.find.mockResolvedValue([{ ...existing, bypass_actors: [actor('OrganizationAdmin', null), actor('DeployKey', null, 'pull_request')] }]) + github.request.mockClear() + expect(await plugin.sync()).toBeUndefined() + expect(plugin.hasChanges).toBe(false) + expect(github.request).not.toHaveBeenCalled() + }) + + it('converges after resolving a team alias alongside an ignored DeployKey id', async () => { + github.rest.teams = { getByName: jest.fn().mockResolvedValue({ data: { id: 42 } }) } + const attrs = policy([{ name: 'developers', actor_type: 'Team', bypass_mode: 'always' }, actor('DeployKey', 123)]) + const plugin = configure([attrs], scope, true) + plugin.find = jest.fn().mockResolvedValue([{ ...policy([actor('DeployKey', null), actor('Team', 42)]), id: 7 }]) + + expect(await plugin.sync()).toBeUndefined() + expect(plugin.hasChanges).toBe(false) + expect(github.rest.teams.getByName).toHaveBeenCalledWith({ org: 'jitran', team_slug: 'developers' }) + expect(github.request).not.toHaveBeenCalled() + }) + }) + describe('idempotent create when the ruleset already exists (retried/concurrent POST)', () => { function duplicateNameError () { const e = new Error('Validation Failed') From d18ef76d9e28391cb4975c3f7e7b72359b037c1d Mon Sep 17 00:00:00 2001 From: Yadhav Jayaraman <57544838+decyjphr@users.noreply.github.com> Date: Mon, 28 Sep 2026 15:00:00 -0400 Subject: [PATCH 2/3] test(smoke): isolate bypass actor convergence fixtures Avoid unrelated property and pull-request-rule default drift in the focused no-op checks. Exercise removal of a known-valid role rather than an API-rejected role ID, preserving exact actor state and no-rewrite assertions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- smoke-test.js | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/smoke-test.js b/smoke-test.js index 4eee74620..73e556036 100644 --- a/smoke-test.js +++ b/smoke-test.js @@ -3201,11 +3201,11 @@ async function phase19BypassActorConvergence () { ['19b', 'reorder and omit ignored ids', [ actor('RepositoryRole', 5), actor('DeployKey', null), actor('OrganizationAdmin', undefined), actor('RepositoryRole', 4) ], false], - ['19c', 'change bypass mode and a real role id', [ - actor('OrganizationAdmin', null, 'pull_request'), actor('DeployKey', null), actor('RepositoryRole', 3), actor('RepositoryRole', 5) + ['19c', 'change bypass mode and remove a real role id', [ + actor('OrganizationAdmin', null, 'pull_request'), actor('DeployKey', null), actor('RepositoryRole', 5) ], true], ['19d', 'converge with concrete ignored ids', [ - actor('RepositoryRole', 5), actor('DeployKey', 9), actor('RepositoryRole', 3), actor('OrganizationAdmin', 8, 'pull_request') + actor('RepositoryRole', 5), actor('DeployKey', 9), actor('OrganizationAdmin', 8, 'pull_request') ], false] ] const sortActors = actors => actors.slice().sort((a, b) => @@ -3222,10 +3222,11 @@ async function phase19BypassActorConvergence () { bypass_actors: actors, rules: [{ type: 'deletion' }] } - const rulesetYaml = require('js-yaml').dump([attrs]) + // Keep unrelated property and pull-request-rule defaults out of NOP checks. + const configYaml = require('js-yaml').dump({ repository: { name: 'test' }, rulesets: [attrs] }) await deleteBranch(ORG, ADMIN_REPO, branch) await createBranch(ORG, ADMIN_REPO, branch) - await createOrUpdateFile(ORG, ADMIN_REPO, `${CONFIG_PATH}/repos/test.yml`, REPO_TEST_YML + rulesetYaml, branch, `${id}: ${description}`) + await createOrUpdateFile(ORG, ADMIN_REPO, `${CONFIG_PATH}/repos/test.yml`, configYaml, branch, `${id}: ${description}`) const pr = await createPR(ORG, ADMIN_REPO, `${id}: bypass actors ${description}`, branch, defaultBranch) await sleep(WEBHOOK_SETTLE_MS) @@ -3255,7 +3256,7 @@ async function phase19BypassActorConvergence () { const actualActors = sortActors(live.bypass_actors.map(entry => actor(entry.actor_type, entry.actor_id, entry.bypass_mode))) return JSON.stringify(actualActors) === JSON.stringify(expectedActors) ? live : null }, { desc: `${id}: exact bypass actors, modes and real role ids to be applied` }) - if (!assert(details !== null, `${id}: exact actor set applied with null ignored IDs and distinct real IDs`)) { + if (!assert(details !== null, `${id}: exact actor set applied with null ignored IDs and expected real IDs`)) { throw new Error(`${id}: bypass actor state did not converge`) } if (previous) { From ef42d4ef8742b567c1a89ce8407057e798a3a247 Mon Sep 17 00:00:00 2001 From: Yadhav Jayaraman <57544838+decyjphr@users.noreply.github.com> Date: Mon, 28 Sep 2026 21:40:46 -0400 Subject: [PATCH 3/3] fix(settings): deduplicate centralized identifier-less bypass actors Key OrganizationAdmin and DeployKey by actor type so centralized entries replace local actors regardless of ignored IDs. Cover all ruleset scopes, centralized mode precedence, payload uniqueness, and convergence. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- README.md | 6 + lib/settings.js | 5 +- test/unit/lib/settings-bypass-actors.test.js | 113 +++++++++++++++++++ 3 files changed, 122 insertions(+), 2 deletions(-) create mode 100644 test/unit/lib/settings-bypass-actors.test.js diff --git a/README.md b/README.md index 13e00039c..a480f2d30 100644 --- a/README.md +++ b/README.md @@ -363,6 +363,12 @@ Notes: > ⚠️ **Warning:** When `{{EXTERNALLY_DEFINED}}` is removed from an existing branch protection rule or ruleset configuration, the status checks in the existing rules in GitHub will revert to the checks that are defined in safe-settings. From this point onwards, all status checks configured through the GitHub UI will be reverted back to the safe-settings configuration. +Entries in the organization-level `centralized_ruleset_bypass_actors` replace +matching actors in individual rulesets. For `OrganizationAdmin` and `DeployKey`, +identity is the actor type alone: GitHub ignores `actor_id`, so omitted, null, +and concrete IDs all refer to the same actor. The centralized entry's +`bypass_mode` takes precedence without adding a duplicate actor. + #### Referencing ruleset bypass actors and reviewers by name Rulesets normally require numeric ids for `bypass_actors[].actor_id` and for the diff --git a/lib/settings.js b/lib/settings.js index bdff9270e..7df70d6f1 100644 --- a/lib/settings.js +++ b/lib/settings.js @@ -169,16 +169,17 @@ function filterActionByChangedNames (action, changedNames) { // --------------------------------------------------------------------------- // Builds a de-duplication key for a bypass actor entry, keyed on actor_type -// plus either actor_id or name (whichever is present). +// alone for identifier-less actors, otherwise plus actor_id or name. function bypassActorKey (actor) { const actorType = actor.actor_type || 'unknown' + if (actorType === 'OrganizationAdmin' || actorType === 'DeployKey') return `actor_type:${actorType}` if (actor.actor_id !== undefined && actor.actor_id !== null) return `actor_id:${actorType}:${actor.actor_id}` if (actor.name) return `name:${actorType}:${actor.name}` return JSON.stringify(actor) } // Merges centrally-declared bypass actors into a ruleset's existing -// bypass_actors, de-duplicating by (actor_type, actor_id|name). Centrally +// bypass_actors, de-duplicating by actor type and any meaningful ID or name. Centrally // declared actors take precedence over a repo/suborg-declared entry with the // same key (e.g. to update bypass_mode). Every entry is shallow-cloned so // the Rulesets plugin's in-place name->id resolution (resolveBypassActor) diff --git a/test/unit/lib/settings-bypass-actors.test.js b/test/unit/lib/settings-bypass-actors.test.js new file mode 100644 index 000000000..017578fde --- /dev/null +++ b/test/unit/lib/settings-bypass-actors.test.js @@ -0,0 +1,113 @@ +const Settings = require('../../../lib/settings') +const Rulesets = require('../../../lib/plugins/rulesets') + +describe('Centralized ruleset bypass actors', () => { + const repo = { owner: 'test-org', repo: 'test-repo' } + const actor = (actorType, actorId, bypassMode = 'always') => ({ + actor_type: actorType, + ...(actorId === undefined ? {} : { actor_id: actorId }), + bypass_mode: bypassMode + }) + let context + + beforeEach(() => { + const request = jest.fn().mockResolvedValue({}) + request.endpoint = jest.fn((url, body) => ({ url, body })) + context = { + payload: { installation: { id: 123 } }, + octokit: { request }, + log: { debug: jest.fn(), info: jest.fn(), error: jest.fn() } + } + }) + + function configure (localActors, centralActors, layer, nop = false) { + const policy = { name: 'Policy', target: 'branch', enforcement: 'active', bypass_actors: localActors, rules: [] } + const config = { centralized_ruleset_bypass_actors: centralActors } + const settings = new Settings(nop, context, repo, config, 'main') + settings.repoConfigs = {} + settings.getSubOrgConfigMap = jest.fn().mockResolvedValue([]) + if (layer === 'repo') settings.repoConfigs = { 'test-repo.yml': { rulesets: [policy] } } + if (layer === 'suborg') settings.subOrgConfigs = { 'test-repo': { rulesets: [policy] } } + if (layer === 'org') config.rulesets = [policy] + if (layer === 'org-suborg') { + settings.getSubOrgConfigMap.mockResolvedValue([{ name: 'suborg.yml', path: '.github/suborgs/suborg.yml' }]) + settings.loadYaml = jest.fn().mockResolvedValue({ + ruleset_scope: 'org', suborgproperties: [{ division: 'engineering' }], rulesets: [policy] + }) + } + return settings + } + + async function run (settings, layer) { + if (layer === 'org' || layer === 'org-suborg') { + await settings.syncOrgLevelRulesets() + } else { + const [Plugin, entries] = settings.childPluginsList(repo).find(([, , section]) => section === 'rulesets') + await new Plugin(settings.nop, context.octokit, repo, entries, context.log, settings.errors).sync() + } + } + + describe.each(['repo', 'suborg', 'org', 'org-suborg'])('%s rulesets', layer => { + it.each([undefined, null, 1, 2].flatMap(localId => + [undefined, null, 1, 2].map(centralId => [localId, centralId]) + ))('replaces ignored local ID %p with centralized ID %p before comparison', async (localId, centralId) => { + const local = ['OrganizationAdmin', 'DeployKey'].map(type => Object.freeze(actor(type, localId))) + const centralized = ['OrganizationAdmin', 'DeployKey'].map(type => Object.freeze(actor(type, centralId, 'pull_request'))) + const settings = configure(Object.freeze(local), Object.freeze(centralized), layer) + let merged + jest.spyOn(Rulesets.prototype, 'find').mockImplementation(function () { + merged = this.entries[0].bypass_actors + return Promise.resolve(this.entries.map(entry => ({ + ...entry, + id: 7, + bypass_actors: ['DeployKey', 'OrganizationAdmin'].map(type => actor(type, null, 'pull_request')) + }))) + }) + + await run(settings, layer) + + expect(merged).toEqual(centralized) + expect(merged).toHaveLength(2) + merged.forEach((entry, index) => expect(entry).not.toBe(centralized[index])) + expect(local).toEqual(['OrganizationAdmin', 'DeployKey'].map(type => actor(type, localId))) + expect(centralized).toEqual(['OrganizationAdmin', 'DeployKey'].map(type => actor(type, centralId, 'pull_request'))) + expect(context.octokit.request).not.toHaveBeenCalled() + expect(settings.errors).toEqual([]) + }) + }) + + it.each([false, true])('applies the centralized mode once, preserves real IDs, and converges (nop=%p)', async nop => { + const local = [actor('DeployKey', null), actor('Team', 42), actor('Team', 43), actor('Integration', 101)] + const centralized = [actor('DeployKey', 99, 'pull_request'), actor('Team', 42, 'pull_request')] + const settings = configure(local, centralized, 'repo', nop) + const [, entries] = settings.childPluginsList(repo).find(([, , section]) => section === 'rulesets') + const expected = [centralized[0], centralized[1], local[2], local[3]] + const plugin = new Rulesets(nop, context.octokit, repo, entries, context.log, settings.errors) + const existing = { ...entries[0], id: 7, bypass_actors: local } + plugin.find = jest.fn().mockResolvedValue([existing]) + + expect(entries[0].bypass_actors).toEqual(expected) + const results = await plugin.sync() + expect(plugin.hasChanges).toBe(true) + if (nop) { + const update = results.flat().find(command => command.action.msg === 'Update Ruleset') + expect(update.body.bypass_actors).toEqual(expected) + expect(context.octokit.request).not.toHaveBeenCalled() + } else { + expect(context.octokit.request).toHaveBeenCalledTimes(1) + expect(context.octokit.request).toHaveBeenCalledWith('PUT /repos/{owner}/{repo}/rulesets/{id}', expect.objectContaining({ + id: 7, bypass_actors: expected + })) + } + + context.octokit.request.mockClear() + plugin.find.mockResolvedValue([{ + ...existing, + bypass_actors: [actor('DeployKey', null, 'pull_request'), centralized[1], local[2], local[3]] + }]) + expect(await plugin.sync()).toBeUndefined() + expect(plugin.hasChanges).toBe(false) + expect(context.octokit.request).not.toHaveBeenCalled() + expect(settings.errors).toEqual([]) + }) +})