From 8d211b0c5045babab754ff150ab5ec188aecf7b8 Mon Sep 17 00:00:00 2001 From: Yadhav Jayaraman <57544838+decyjphr@users.noreply.github.com> Date: Thu, 30 Jul 2026 10:27:00 -0400 Subject: [PATCH 1/4] Incorporate PR #1010: modernize security manager team handling Bring PR #1010 (bug/issue-903) into this branch. The comprehensive teams.js here still used the deprecated GET /orgs/{org}/security-managers endpoint, so the security-manager modernization was not yet incorporated. - teams.js: identify security manager teams via the organization roles API (GET /orgs/{org}/organization-roles and .../{role_id}/teams) instead of the deprecated security-managers endpoint. Adapted to this branch's non-`rest` Octokit client convention (this.github.repos/teams.*). - Add team name/slug and role name normalization helpers so configured names match existing slugs without add/remove churn. - Add skipTeamDeletion guard: if security manager discovery fails, keep repository teams unchanged instead of deleting them. - app.yml already grants organization_custom_roles (write), so no permission change needed; document the org "Custom organization roles" permission in docs/deploy.md. - Port unit + integration test coverage for the new behavior. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 64c2ef45-ed6c-4756-9ec6-58a35797dc0e --- docs/deploy.md | 1 + lib/plugins/teams.js | 109 +++++++++---- test/integration/plugins/teams.test.js | 15 +- test/unit/lib/plugins/teams.test.js | 206 ++++++++++++++++++++++++- 4 files changed, 297 insertions(+), 34 deletions(-) diff --git a/docs/deploy.md b/docs/deploy.md index d61377bed..7767899a8 100644 --- a/docs/deploy.md +++ b/docs/deploy.md @@ -295,6 +295,7 @@ Every deployment will need an [App](https://developer.github.com/apps/). #### Organization Permissions - Administration: **Read & Write** +- Custom organization roles: **Read-only** - Custom properties: **Admin** - Members: **Read & Write** diff --git a/lib/plugins/teams.js b/lib/plugins/teams.js index 0df459a2f..707b9ec5f 100644 --- a/lib/plugins/teams.js +++ b/lib/plugins/teams.js @@ -4,6 +4,8 @@ const NopCommand = require('../nopcommand') const teamRepoEndpoint = '/orgs/:owner/teams/:team_slug/repos/:owner/:repo' const listExternalGroupsEndpoint = 'GET /orgs/{org}/external-groups' const teamExternalGroupsEndpoint = '/orgs/{org}/teams/{team_slug}/external-groups' +const securityManagerRoleName = 'security_manager' +const safeSecurityManagerStatuses = [403, 404, 422] module.exports = class Teams extends Diffable { // Override Diffable.sync to also reconcile the optional `external_group` @@ -27,6 +29,7 @@ module.exports = class Teams extends Diffable { } async find () { + this.skipTeamDeletion = false this.log.debug(`Finding teams for ${this.repo.owner}/${this.repo.repo}`) return this.github.paginate(this.github.repos.listTeams, this.repo).then(res => { this.log.debug(`Found teams ${JSON.stringify(res)}`) @@ -37,47 +40,90 @@ module.exports = class Teams extends Diffable { // remove all security manager teams async checkSecurityManager (teams) { try { - // Uncomment the following lines to handle the deprecation of the teams api https://gh.io/security-managers-rest-api-sunset - // but this would require a new permission on the app - // - // const roles = await this.github.paginate('GET /orgs/{org}/roles', { org: this.repo.owner }) - // const securityManagerRole = roles.find(role => role.name === 'security_manager') - // - // this.log.debug(`Calling API to get security managers ${JSON.stringify(this.github.request.endpoint('GET /orgs/{org}/roles/{role_id}/teams', - // { - // org: this.repo.owner, - // role_id: securityManagerRole.id - // }))} `) - // const resp = await this.github.paginate('GET /orgs/{org}/roles/{role_id}/teams', - // { - // org: this.repo.owner, - // role_id: securityManagerRole.id - // }) - this.log.debug('Removing all security manager teams since they should not be handled here') - this.log.debug(`Calling API to get security managers ${JSON.stringify(this.github.request.endpoint('GET /orgs/{org}/security-managers', - { - org: this.repo.owner - }))} `) - const resp = await this.github.paginate('GET /orgs/{org}/security-managers', + this.log.debug(`Calling API to get organization roles ${JSON.stringify(this.github.request.endpoint('GET /orgs/{org}/organization-roles', + { + org: this.repo.owner + }))} `) + const rolesResp = await this.github.paginate('GET /orgs/{org}/organization-roles', { org: this.repo.owner }) + const roles = this.toArray(rolesResp, 'roles') + const securityManagerRole = roles.find(role => this.isSecurityManagerRole(role)) + + if (!securityManagerRole || !securityManagerRole.id) { + this.log.debug(`${this.repo.owner} Org does not have a security manager organization role set up`) + return teams + } + + const params = { + org: this.repo.owner, + role_id: securityManagerRole.id + } + this.log.debug(`Calling API to get security manager teams ${JSON.stringify(this.github.request.endpoint('GET /orgs/{org}/organization-roles/{role_id}/teams', params))} `) + const resp = await this.github.paginate('GET /orgs/{org}/organization-roles/{role_id}/teams', params) this.log.debug(`Response from the call is ${JSON.stringify(resp)}`) - return teams.filter(team => !resp.some(sec => sec.name === team.name)) + const securityManagerTeams = this.toArray(resp, 'teams') + const securityManagerTeamIdentifiers = new Set(securityManagerTeams.flatMap(team => [team.slug, team.name].map(name => this.normalizeTeamIdentifier(name))).filter(Boolean)) + + return teams.filter(team => !this.isSecurityManagerTeam(team, securityManagerTeamIdentifiers)) } catch (e) { - if (e.status === 404) { - this.log.debug(`${this.repo.owner} Org does not have Security manager teams set up ${e}`) + this.skipTeamDeletion = true + const status = e && e.status + if (safeSecurityManagerStatuses.includes(status)) { + this.log.debug(`${this.repo.owner} Org security manager teams could not be fetched with status ${status}; keeping repository teams unchanged ${e}`) } else { this.log.error( - `Unexpected error when fetching for security manager teams org ${this.repo.owner} = ${e}` + `Unexpected error when fetching security manager teams for org ${this.repo.owner}; keeping repository teams unchanged ${e}` ) } return teams } } + toArray (resp, propertyName) { + if (Array.isArray(resp)) { + return resp + } + + if (resp && Array.isArray(resp[propertyName])) { + return resp[propertyName] + } + + return [] + } + + isSecurityManagerRole (role) { + return [role && role.name, role && role.slug] + .map(name => this.normalizeRoleName(name)) + .includes(securityManagerRoleName) + } + + normalizeRoleName (name) { + if (typeof name !== 'string') { + return '' + } + + return name.trim().toLowerCase().replace(/[\s-]+/g, '_') + } + + normalizeTeamIdentifier (name) { + if (typeof name !== 'string') { + return '' + } + + return name.trim().toLowerCase().replace(/['’]/g, '').replace(/[^a-z0-9]+/g, '-').replace(/^-+|-+$/g, '') + } + + isSecurityManagerTeam (team, securityManagerTeamIdentifiers) { + return [team.slug, team.name] + .map(name => this.normalizeTeamIdentifier(name)) + .filter(Boolean) + .some(name => securityManagerTeamIdentifiers.has(name)) + } + comparator (existing, attrs) { - return existing.slug === attrs.name.toLowerCase() + return this.normalizeTeamIdentifier(existing.slug || existing.name) === this.normalizeTeamIdentifier(attrs.name) } changed (existing, attrs) { @@ -96,7 +142,7 @@ module.exports = class Teams extends Diffable { add (attrs) { let existing = { team_id: 1 } this.log.debug(`Getting team with the parms ${JSON.stringify(attrs)}`) - return this.github.teams.getByName({ org: this.repo.owner, team_slug: attrs.name }).then(res => { + return this.github.teams.getByName({ org: this.repo.owner, team_slug: this.normalizeTeamIdentifier(attrs.name) }).then(res => { existing = res.data this.log.debug(`adding team ${attrs.name} to repo ${this.repo.repo}`) if (this.nop) { @@ -137,6 +183,11 @@ module.exports = class Teams extends Diffable { } remove (existing) { + if (this.skipTeamDeletion) { + this.log.debug(`Skipping deletion of team ${existing.slug} from repo ${this.repo.repo} because security manager team discovery failed`) + return Promise.resolve() + } + if (this.nop) { return Promise.resolve([ new NopCommand(this.constructor.name, this.repo, this.github.request.endpoint( @@ -155,7 +206,7 @@ module.exports = class Teams extends Diffable { return { team_id: existing.id, org: this.repo.owner, - team_slug: attrs.name, + team_slug: existing.slug || this.normalizeTeamIdentifier(attrs.name), owner: this.repo.owner, repo: this.repo.repo, permission: attrs.permission diff --git a/test/integration/plugins/teams.test.js b/test/integration/plugins/teams.test.js index 4fde0637f..535a510f2 100644 --- a/test/integration/plugins/teams.test.js +++ b/test/integration/plugins/teams.test.js @@ -24,6 +24,8 @@ describe('teams plugin', function () { const probotTeamId = any.integer() const greenkeeperKeeperTeamId = any.integer() const formationTeamId = any.integer() + const securityManagerRoleId = any.integer() + const securityManagerTeamId = any.integer() githubScope .get(`/repos/${repository.owner.name}/${repository.name}/contents/${settings.FILE_PATH}`) .reply(OK, { content: encodedConfig, name: 'settings.yml', type: 'file' }) @@ -33,9 +35,20 @@ describe('teams plugin', function () { OK, [ { slug: 'greenkeeper-keeper', id: greenkeeperKeeperTeamId, permission: 'pull' }, - { slug: 'form8ion', id: formationTeamId, permission: 'push' } + { slug: 'form8ion', id: formationTeamId, permission: 'push' }, + { slug: 'security-managers', id: securityManagerTeamId, permission: 'push' } ] ) + githubScope + .get(`/orgs/${repository.owner.name}/organization-roles`) + .reply(OK, { + roles: [{ id: securityManagerRoleId, slug: 'security_manager', name: 'Security Manager' }] + }) + githubScope + .get(`/orgs/${repository.owner.name}/organization-roles/${securityManagerRoleId}/teams`) + .reply(OK, { + teams: [{ id: securityManagerTeamId, slug: 'security-managers', name: 'Security Managers' }] + }) githubScope .get(`/orgs/${repository.owner.name}/teams/probot`) .reply(OK, { id: probotTeamId }) diff --git a/test/unit/lib/plugins/teams.test.js b/test/unit/lib/plugins/teams.test.js index 8879611c3..e097e2325 100644 --- a/test/unit/lib/plugins/teams.test.js +++ b/test/unit/lib/plugins/teams.test.js @@ -23,9 +23,12 @@ describe('Teams', () => { beforeEach(() => { github = { paginate: jest.fn() - .mockImplementation(async (fetch) => { - const response = await fetch() - return response.data + .mockImplementation(async (fetchOrRoute) => { + if (typeof fetchOrRoute === 'function') { + const response = await fetchOrRoute() + return response.data + } + return [] }), teams: { create: jest.fn().mockResolvedValue(), @@ -41,7 +44,7 @@ describe('Teams', () => { ] }) }, - request: jest.fn().mockResolvedValue() + request: Object.assign(jest.fn().mockResolvedValue(), { endpoint: jest.fn().mockReturnValue('endpoint-stub') }) } }) @@ -97,6 +100,201 @@ describe('Teams', () => { } }) + describe('security manager teams', () => { + const securityManagerRoleId = any.integer() + const securityManagerTeamName = 'security-managers' + const securityManagerTeamId = any.integer() + const organizationRolesRoute = 'GET /orgs/{org}/organization-roles' + const organizationRoleTeamsRoute = 'GET /orgs/{org}/organization-roles/{role_id}/teams' + const roleFailureStatuses = [403, 404, 422, 500] + const repoTeams = [ + { id: securityManagerTeamId, slug: securityManagerTeamName, name: 'Security Managers', permission: 'admin' }, + { id: unchangedTeamId, slug: unchangedTeamName, permission: 'push' }, + { id: removedTeamId, slug: removedTeamName, permission: 'push' }, + { id: updatedTeamId, slug: updatedTeamName, permission: 'pull' } + ] + + beforeEach(() => { + github.repos.listTeams.mockResolvedValue({ data: repoTeams }) + }) + + function expectTeamDeleted (teamSlug) { + expect(github.request).toHaveBeenCalledWith( + 'DELETE /orgs/:owner/teams/:team_slug/repos/:owner/:repo', + { + org, + owner: org, + repo: 'test', + team_slug: teamSlug + } + ) + } + + function expectTeamNotDeleted (teamSlug) { + expect(github.request).not.toHaveBeenCalledWith( + 'DELETE /orgs/:owner/teams/:team_slug/repos/:owner/:repo', + { + org, + owner: org, + repo: 'test', + team_slug: teamSlug + } + ) + } + + function expectNoTeamsDeleted () { + expect(github.request).not.toHaveBeenCalledWith( + 'DELETE /orgs/:owner/teams/:team_slug/repos/:owner/:repo', + expect.any(Object) + ) + } + + it('syncs non-security-manager teams and leaves security manager teams untouched', async () => { + const plugin = configure([ + { name: unchangedTeamName, permission: 'push' }, + { name: updatedTeamName, permission: 'admin' }, + { name: addedTeamName, permission: 'pull' } + ]) + + when(github.paginate) + .calledWith(organizationRolesRoute, { org }) + .mockResolvedValue({ roles: [{ id: securityManagerRoleId, name: 'Security Manager' }] }) + + when(github.paginate) + .calledWith(organizationRoleTeamsRoute, { org, role_id: securityManagerRoleId }) + .mockResolvedValue({ teams: [{ slug: securityManagerTeamName, name: 'Security Managers' }] }) + + when(github.teams.getByName) + .defaultResolvedValue({}) + .calledWith({ org, team_slug: addedTeamName }) + .mockResolvedValue({ data: { id: addedTeamId } }) + + await plugin.sync() + + expect(github.paginate).toHaveBeenCalledWith(organizationRolesRoute, { org }) + expect(github.paginate).toHaveBeenCalledWith(organizationRoleTeamsRoute, { org, role_id: securityManagerRoleId }) + expectTeamDeleted(removedTeamName) + expectTeamNotDeleted(securityManagerTeamName) + }) + + it.each(roleFailureStatuses)('skips deletions when organization role lookup fails with %s', async status => { + const plugin = configure([ + { name: unchangedTeamName, permission: 'push' } + ]) + + when(github.paginate) + .calledWith(organizationRolesRoute, { org }) + .mockRejectedValue({ status }) + + await plugin.sync() + + expectNoTeamsDeleted() + }) + + it.each(roleFailureStatuses)('skips deletions when organization role team lookup fails with %s', async status => { + const plugin = configure([ + { name: unchangedTeamName, permission: 'push' } + ]) + + when(github.paginate) + .calledWith(organizationRolesRoute, { org }) + .mockResolvedValue({ roles: [{ id: securityManagerRoleId, slug: 'security_manager' }] }) + + when(github.paginate) + .calledWith(organizationRoleTeamsRoute, { org, role_id: securityManagerRoleId }) + .mockRejectedValue({ status }) + + await plugin.sync() + + expectNoTeamsDeleted() + }) + + it('matches configured team names to existing slugs without add or remove churn', async () => { + const formattedTeamName = 'Platform & Security!' + + github.repos.listTeams.mockResolvedValue({ + data: [{ id: unchangedTeamId, slug: 'platform-security', name: formattedTeamName, permission: 'push' }] + }) + + const plugin = configure([ + { name: formattedTeamName, permission: 'push' } + ]) + + await plugin.sync() + + expect(github.teams.getByName).not.toHaveBeenCalled() + expectNoTeamsDeleted() + }) + + it('matches security manager team names against repository team slugs', async () => { + github.repos.listTeams.mockResolvedValue({ + data: [{ id: securityManagerTeamId, slug: securityManagerTeamName, permission: 'admin' }] + }) + + when(github.paginate) + .calledWith(organizationRolesRoute, { org }) + .mockResolvedValue({ roles: [{ id: securityManagerRoleId, name: 'Security Manager' }] }) + + when(github.paginate) + .calledWith(organizationRoleTeamsRoute, { org, role_id: securityManagerRoleId }) + .mockResolvedValue({ teams: [{ name: 'Security Managers' }] }) + + const plugin = configure([]) + + await expect(plugin.find()).resolves.toEqual([]) + }) + + it('uses normalized team slugs when adding configured team names', async () => { + const formattedTeamName = 'Platform & Security!' + + github.repos.listTeams.mockResolvedValue({ data: [] }) + + when(github.teams.getByName) + .calledWith({ org, team_slug: 'platform-security' }) + .mockResolvedValue({ data: { id: addedTeamId, slug: 'platform-security' } }) + + const plugin = configure([ + { name: formattedTeamName, permission: 'pull' } + ]) + + await plugin.sync() + + expect(github.teams.addOrUpdateRepoPermissionsInOrg).toHaveBeenCalledWith({ + org, + team_id: addedTeamId, + team_slug: 'platform-security', + owner: org, + repo: 'test', + permission: 'pull' + }) + }) + + it('returns original teams when the security manager role is absent', async () => { + const plugin = configure([]) + + when(github.paginate) + .calledWith(organizationRolesRoute, { org }) + .mockResolvedValue({ roles: [{ id: any.integer(), name: 'compliance_manager' }] }) + + await expect(plugin.find()).resolves.toEqual(repoTeams) + expect(github.paginate).not.toHaveBeenCalledWith(organizationRoleTeamsRoute, { org, role_id: securityManagerRoleId }) + }) + + it('returns original teams when organization role team lookup fails', async () => { + const plugin = configure([]) + + when(github.paginate) + .calledWith(organizationRolesRoute, { org }) + .mockResolvedValue({ roles: [{ id: securityManagerRoleId, slug: 'security_manager' }] }) + + when(github.paginate) + .calledWith(organizationRoleTeamsRoute, { org, role_id: securityManagerRoleId }) + .mockRejectedValue({ status: 500 }) + + await expect(plugin.find()).resolves.toEqual(repoTeams) + }) + }) + describe('external_group linking', () => { const externalGroupName = 'Engineering - Expert Services' const externalGroupId = 42 From 70c5f1eeb8ba098469e658f79e304e73c8585253 Mon Sep 17 00:00:00 2001 From: Yadhav Jayaraman <57544838+decyjphr@users.noreply.github.com> Date: Tue, 4 Aug 2026 15:16:43 -0400 Subject: [PATCH 2/4] fix(teams): never add/update/remove security manager teams from config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit checkSecurityManager() only filtered security manager teams out of the existing repo team list, which suppressed deletes/updates when they were absent from config. But a config entry naming a security manager team then looked "missing" to Diffable.sync(), so add()/addOrUpdateRepoPermissionsInOrg still fired — letting this plugin modify security manager teams, contrary to the "should not be handled here" intent. Persist the discovered security manager team identifiers on the instance and no-op add(), update(), and remove() when the configured team matches them. In nop mode an INFO command is emitted so PR reviewers see the skip. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 64c2ef45-ed6c-4756-9ec6-58a35797dc0e --- lib/plugins/teams.js | 38 +++++++++++++++++++++++ test/unit/lib/plugins/teams.test.js | 47 +++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+) diff --git a/lib/plugins/teams.js b/lib/plugins/teams.js index 707b9ec5f..6680bddf4 100644 --- a/lib/plugins/teams.js +++ b/lib/plugins/teams.js @@ -30,6 +30,7 @@ module.exports = class Teams extends Diffable { async find () { this.skipTeamDeletion = false + this.securityManagerTeamIdentifiers = new Set() this.log.debug(`Finding teams for ${this.repo.owner}/${this.repo.repo}`) return this.github.paginate(this.github.repos.listTeams, this.repo).then(res => { this.log.debug(`Found teams ${JSON.stringify(res)}`) @@ -65,6 +66,11 @@ module.exports = class Teams extends Diffable { this.log.debug(`Response from the call is ${JSON.stringify(resp)}`) const securityManagerTeams = this.toArray(resp, 'teams') const securityManagerTeamIdentifiers = new Set(securityManagerTeams.flatMap(team => [team.slug, team.name].map(name => this.normalizeTeamIdentifier(name))).filter(Boolean)) + // Persist the identifiers so add()/update()/remove() can no-op for + // security manager teams even when they appear in the config. Without + // this, a configured security manager team would look "missing" from the + // filtered existing list and Diffable.sync() would (re)add it here. + this.securityManagerTeamIdentifiers = securityManagerTeamIdentifiers return teams.filter(team => !this.isSecurityManagerTeam(team, securityManagerTeamIdentifiers)) } catch (e) { @@ -122,6 +128,28 @@ module.exports = class Teams extends Diffable { .some(name => securityManagerTeamIdentifiers.has(name)) } + // True when the given attrs/record refers to a discovered security manager + // team. Security manager teams are intentionally not managed by this plugin, + // so add()/update()/remove() must no-op for them even if they are present in + // the config file. + isConfiguredSecurityManagerTeam (attrs) { + if (!this.securityManagerTeamIdentifiers || this.securityManagerTeamIdentifiers.size === 0) { + return false + } + return this.isSecurityManagerTeam(attrs, this.securityManagerTeamIdentifiers) + } + + skipSecurityManagerTeam (attrs, verb) { + const teamName = (attrs && (attrs.name || attrs.slug)) || 'unknown' + this.log.debug(`Skipping ${verb} of security manager team ${teamName} for repo ${this.repo.repo}; security manager teams are not managed here`) + if (this.nop) { + return Promise.resolve([ + new NopCommand(this.constructor.name, this.repo, null, `Skipping ${verb} of security manager team ${teamName}; security manager teams are not managed by safe-settings`, 'INFO') + ]) + } + return Promise.resolve() + } + comparator (existing, attrs) { return this.normalizeTeamIdentifier(existing.slug || existing.name) === this.normalizeTeamIdentifier(attrs.name) } @@ -131,6 +159,9 @@ module.exports = class Teams extends Diffable { } update (existing, attrs) { + if (this.isConfiguredSecurityManagerTeam(attrs)) { + return this.skipSecurityManagerTeam(attrs, 'update') + } if (this.nop) { return Promise.resolve([ new NopCommand(this.constructor.name, this.repo, this.github.request.endpoint(`PUT ${teamRepoEndpoint}`, this.toParams(existing, attrs)), 'Add Teams to Repo') @@ -140,6 +171,9 @@ module.exports = class Teams extends Diffable { } add (attrs) { + if (this.isConfiguredSecurityManagerTeam(attrs)) { + return this.skipSecurityManagerTeam(attrs, 'add') + } let existing = { team_id: 1 } this.log.debug(`Getting team with the parms ${JSON.stringify(attrs)}`) return this.github.teams.getByName({ org: this.repo.owner, team_slug: this.normalizeTeamIdentifier(attrs.name) }).then(res => { @@ -183,6 +217,10 @@ module.exports = class Teams extends Diffable { } remove (existing) { + if (this.isConfiguredSecurityManagerTeam(existing)) { + return this.skipSecurityManagerTeam(existing, 'removal') + } + if (this.skipTeamDeletion) { this.log.debug(`Skipping deletion of team ${existing.slug} from repo ${this.repo.repo} because security manager team discovery failed`) return Promise.resolve() diff --git a/test/unit/lib/plugins/teams.test.js b/test/unit/lib/plugins/teams.test.js index e097e2325..9a1d5b436 100644 --- a/test/unit/lib/plugins/teams.test.js +++ b/test/unit/lib/plugins/teams.test.js @@ -177,6 +177,53 @@ describe('Teams', () => { expectTeamNotDeleted(securityManagerTeamName) }) + it('does not add or update a security manager team even when it is listed in the config', async () => { + const plugin = configure([ + { name: securityManagerTeamName, permission: 'pull' }, + { name: unchangedTeamName, permission: 'push' } + ]) + + when(github.paginate) + .calledWith(organizationRolesRoute, { org }) + .mockResolvedValue({ roles: [{ id: securityManagerRoleId, name: 'Security Manager' }] }) + + when(github.paginate) + .calledWith(organizationRoleTeamsRoute, { org, role_id: securityManagerRoleId }) + .mockResolvedValue({ teams: [{ slug: securityManagerTeamName, name: 'Security Managers' }] }) + + await plugin.sync() + + expect(github.teams.getByName).not.toHaveBeenCalledWith({ org, team_slug: securityManagerTeamName }) + expect(github.teams.addOrUpdateRepoPermissionsInOrg).not.toHaveBeenCalled() + expect(github.request).not.toHaveBeenCalledWith( + 'PUT /orgs/:owner/teams/:team_slug/repos/:owner/:repo', + expect.objectContaining({ team_slug: securityManagerTeamName }) + ) + expectTeamNotDeleted(securityManagerTeamName) + }) + + it('emits an INFO nop command instead of managing a configured security manager team in nop mode', async () => { + const log = { debug: jest.fn(), error: jest.fn(), warn: jest.fn() } + const plugin = new Teams(true, github, { owner: org, repo: 'test' }, [ + { name: securityManagerTeamName, permission: 'pull' } + ], log, []) + + when(github.paginate) + .calledWith(organizationRolesRoute, { org }) + .mockResolvedValue({ roles: [{ id: securityManagerRoleId, name: 'Security Manager' }] }) + + when(github.paginate) + .calledWith(organizationRoleTeamsRoute, { org, role_id: securityManagerRoleId }) + .mockResolvedValue({ teams: [{ slug: securityManagerTeamName, name: 'Security Managers' }] }) + + const result = await plugin.sync() + + expect(Array.isArray(result)).toBe(true) + const flattened = result.flat(Infinity) + expect(flattened.some(c => c && c.type === 'INFO' && /security manager team/i.test(JSON.stringify(c)))).toBe(true) + expect(github.teams.addOrUpdateRepoPermissionsInOrg).not.toHaveBeenCalled() + }) + it.each(roleFailureStatuses)('skips deletions when organization role lookup fails with %s', async status => { const plugin = configure([ { name: unchangedTeamName, permission: 'push' } From 9e6d204ade93e3127256ab00f48e8515dba2453e Mon Sep 17 00:00:00 2001 From: Yadhav Jayaraman <57544838+decyjphr@users.noreply.github.com> Date: Tue, 4 Aug 2026 16:17:24 -0400 Subject: [PATCH 3/4] test(teams): make request.endpoint mock return Octokit-shaped object The mock returned a bare string, but production Octokit's request.endpoint() returns an object with url/body. NopCommand reads endpoint.url and endpoint.body, so the string mock silently produced undefined values and reduced test fidelity. Return { url, body } to match the real shape. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 64c2ef45-ed6c-4756-9ec6-58a35797dc0e --- test/unit/lib/plugins/teams.test.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/unit/lib/plugins/teams.test.js b/test/unit/lib/plugins/teams.test.js index dd5b46091..dbd3a80f9 100644 --- a/test/unit/lib/plugins/teams.test.js +++ b/test/unit/lib/plugins/teams.test.js @@ -44,7 +44,7 @@ describe('Teams', () => { ] }) }, - request: Object.assign(jest.fn().mockResolvedValue(), { endpoint: jest.fn().mockReturnValue('endpoint-stub') }) + request: Object.assign(jest.fn().mockResolvedValue(), { endpoint: jest.fn().mockReturnValue({ url: 'endpoint-stub', body: {} }) }) } }) @@ -403,7 +403,7 @@ describe('Teams', () => { } return Promise.resolve({ data: {} }) }) - github.request.endpoint = jest.fn().mockReturnValue('endpoint-stub') + github.request.endpoint = jest.fn().mockReturnValue({ url: 'endpoint-stub', body: {} }) // paginate: route the external-groups list call to a single page; keep // the original implementation for other paginated endpoints. The real @@ -466,7 +466,7 @@ describe('Teams', () => { } return Promise.resolve({ data: {} }) }) - github.request.endpoint = jest.fn().mockReturnValue('endpoint-stub') + github.request.endpoint = jest.fn().mockReturnValue({ url: 'endpoint-stub', body: {} }) const plugin = configure([ { name: unchangedTeamName, permission: 'push', external_group: externalGroupName } From 17c3239d41880d8a78f69b4e9e4a222b177b8327 Mon Sep 17 00:00:00 2001 From: Yadhav Jayaraman <57544838+decyjphr@users.noreply.github.com> Date: Tue, 4 Aug 2026 16:20:10 -0400 Subject: [PATCH 4/4] fix(teams): emit INFO NopCommand when skipping deletion after SM discovery failure When security-manager discovery fails, remove() sets skipTeamDeletion and returned a bare Promise.resolve() even in nop mode. Diffable.sync() pushed that undefined into the nop command list, hiding the fact that a deletion was intentionally skipped. Return an INFO NopCommand in nop mode so the dry-run output is accurate and no undefined entries accumulate. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 64c2ef45-ed6c-4756-9ec6-58a35797dc0e --- lib/plugins/teams.js | 8 +++++++- test/unit/lib/plugins/teams.test.js | 18 ++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/lib/plugins/teams.js b/lib/plugins/teams.js index 6680bddf4..5e789526a 100644 --- a/lib/plugins/teams.js +++ b/lib/plugins/teams.js @@ -222,7 +222,13 @@ module.exports = class Teams extends Diffable { } if (this.skipTeamDeletion) { - this.log.debug(`Skipping deletion of team ${existing.slug} from repo ${this.repo.repo} because security manager team discovery failed`) + const msg = `Skipping deletion of team ${existing.slug} from repo ${this.repo.repo} because security manager team discovery failed` + this.log.debug(msg) + if (this.nop) { + return Promise.resolve([ + new NopCommand(this.constructor.name, this.repo, null, msg, 'INFO') + ]) + } return Promise.resolve() } diff --git a/test/unit/lib/plugins/teams.test.js b/test/unit/lib/plugins/teams.test.js index dbd3a80f9..5e46a3654 100644 --- a/test/unit/lib/plugins/teams.test.js +++ b/test/unit/lib/plugins/teams.test.js @@ -256,6 +256,24 @@ describe('Teams', () => { expectNoTeamsDeleted() }) + it('emits an INFO nop command when skipping deletion in nop mode after discovery failure', async () => { + const log = { debug: jest.fn(), error: jest.fn(), warn: jest.fn() } + const plugin = new Teams(true, github, { owner: org, repo: 'test' }, [ + { name: unchangedTeamName, permission: 'push' } + ], log, []) + + when(github.paginate) + .calledWith(organizationRolesRoute, { org }) + .mockRejectedValue({ status: 500 }) + + const result = await plugin.sync() + + expect(Array.isArray(result)).toBe(true) + const flattened = result.flat(Infinity) + expect(flattened.some(c => c && c.type === 'INFO' && /security manager team discovery failed/i.test(JSON.stringify(c)))).toBe(true) + expectNoTeamsDeleted() + }) + it('matches configured team names to existing slugs without add or remove churn', async () => { const formattedTeamName = 'Platform & Security!'