Teams plugin: PR comment/nop diff shows phantom additions+deletions for unchanged teams due to name vs. slug mismatch

[StephHope]

Problem Description

What is actually happening

When running in nop mode (dry-run PR comments), the Teams plugin frequently reports every team on a repo as a full addition and a full deletion in the same diff, even when the configured permission genuinely has not changed and no API calls end up being made.

This happens because two separate code paths use different keys to identify a team:

• The actual sync logic in lib/plugins/teams.js matches an existing team-repo record to a config entry by slug:

  comparator (existing, attrs) {
    return existing.slug === attrs.name.toLowerCase()
  }

  and only treats permission as a real, actionable difference (changed()). This is correct and works fine.

• The nop/PR-comment diff table, generated separately via MergeDeep.compareDeep() (called from lib/plugins/diffable.js), pairs array items using GET_NAME_USERNAME_PROPERTY in lib/mergeDeep.js, which always prefers the object's name property. GitHub's team API objects have name set to the display name (e.g. "Platform Engineering"), which is distinct from slug (e.g. "platform-engineering"). Config entries use name to mean the slug (per the comparator() convention above). When the display name and slug don't textually match (multi-word team names, different casing, etc.), compareDeep's pairing fails to recognize the config entry and the existing GitHub record as "the same team" — so it reports the entire existing record as a deletion and the entire config entry as an addition, even though nothing is actually going to change.

This also causes irrelevant/unsettable fields (e.g. notification_setting, which is part of the raw GitHub team object but is never read or sent by teams.js's toParams()/add()/update()) to show up in the diff, making the PR comment noisy and hard to review for the changes that do matter.

What is the expected behavior

For teams whose permission is unchanged, the nop/PR comment diff should show no changes for that team — consistent with what the real sync logic (comparator/changed) determines. The diff table should match on the same identity (slug) that the actual sync uses, not on GitHub's display name.

Example

Given a repo config:

teams:
  - name: platform-engineering
    permission: push

and an existing GitHub team already attached to the repo with slug platform-engineering, display name "Platform Engineering", and permission push (i.e., truly no change needed), the PR comment nonetheless shows:

• Additions: { name: platform-engineering, permission: push }
• Deletions: the full existing team object, including fields like notification_setting: notifications_enabled that aren't part of the config at all and aren't touched by any API call.

Suggested Fix

Two approaches, in increasing order of scope/risk:

Option A — normalize the identity field in Teams.find() (small, isolated change)

Before existing team-repo records are handed to the shared diff machinery, overwrite their name with slug:

async find () {
  return this.github.paginate(this.github.rest.repos.listTeams, this.repo).then(res => {
    const normalized = res.map(t => ({ ...t, name: t.slug }))
    return this.checkSecurityManager(normalized)
  })
}

Since comparator(), changed(), update(), add(), and remove() all key off existing.slug/existing.id and never read existing.name, this is safe — it only affects what compareDeep sees for display purposes. The change is contained entirely to teams.js; mergeDeep.js and diffable.js (shared by every other plugin) are untouched, so there's no risk of regressing branches/collaborators/rulesets/etc.

Option B — pluggable match-key in MergeDeep/Diffable (more general, larger surface)

Add an optional key-extraction callback to MergeDeep's constructor (defaulting to today's GET_NAME_USERNAME_PROPERTY behavior), and let Diffable subclasses override it. Teams would supply item => item.slug. This generalizes the fix for any future plugin with a similar "config-friendly identifier differs from the API's display name" shape, but touches shared code used by every plugin, so it needs broader regression testing across the whole plugin test suite.

Given this is currently a single known instance of the problem, Option A seems like the pragmatic first step; Option B could be worth revisiting if the same class of bug shows up elsewhere (e.g. any other resource where GitHub's API returns a human-readable name distinct from the identifier used in config).

Context

Are you using the hosted instance of probot/settings or running your own?

Running our own instance (fork of safe-settings).

If running your own instance, are you using it with github.com or GitHub Enterprise?

github.com

Version of probot/settings

2.1.20-rc.3 (main-enterprise @ 4578aea)

Version of GitHub Enterprise

N/A