github-community-projects / github-community-projects/safe-settings
Teams plugin: PR comment/nop diff shows phantom additions+deletions for unchanged teams due to name vs. slug mismatch
- Dominant language
- JavaScript
- Stars
- 921
- Forks
- 226
- Avg merge
- 18h 3m
- Merged PRs (30d)
- 14
Description
## 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`](https://github.com/github/safe-settings/blob/main/lib/plugins/teams.js) matches an existing team-repo record to a config entry by **slug**:
```js
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`](https://github.com/github/safe-settings/blob/main/lib/plugins/diffable.js#L72)), pairs array items using `GET_NAME_USERNAME_PROPERTY` in [`lib/mergeDeep.js`](https://github.com/github/safe-settings/blob/main/lib/mergeDeep.js#L4-L6), 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:
```yaml
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`:
```js
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
Contributor guide
Research direction
Start in lib/plugins/teams.js, especially find() and the comparator used by the sync path, then read how lib/plugins/diffable.js invokes lib/mergeDeep.js for nop output. Verify the change preserves slug-based identity and that an unchanged team produces no additions or deletions in the PR comment while actionable permission changes remain visible.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- javascript
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 76/100