Fix: Patch comparison for Teams plugin to match on slug instead of name on NOP - #3
Open
StephHope wants to merge 1 commit into
Open
Fix: Patch comparison for Teams plugin to match on slug instead of name on NOP#3StephHope wants to merge 1 commit into
StephHope wants to merge 1 commit into
Conversation
pvandervelde
approved these changes
Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the Teams plugin's nop-mode PR comment showing phantom full add+delete diffs for teams that haven't actually changed, and adds a regression test guarding against it.
See github-community-projects#1033
Teams.find()now normalizes each existing team'snameto itsslugbefore the record reaches the shared diff engine (MergeDeep.compareDeep). GitHub's team API returnsnameas the display name (e.g. "Platform Engineering"), while config entries and the plugin's owncomparator()/changed()treatnameas the slug (e.g.platform-engineering). When those two didn't match textually,compareDeepfailed to recognize the config entry and the existing team as the same object, and reported the entire existing record as a deletion and the entire config entry as an addition — even when the real sync logic correctly determined nothing needed to change. This also surfaced irrelevant fields (e.g.notification_setting) that the plugin never reads or sends to the GitHub API.checkSecurityManager(), since that filter matches on the real display name.Why a separate test file
test/unit/lib/plugins/teams.slug-fix.test.jsis intentionally its own file rather than an addition toteams.test.js. This keeps the regression isolated from upstream's own test file, so a future upstream integration merge ontogallagherwon't silently clobber it, and if a merge ever does reintroduce the unpatchedteams.js, this test fails loudly in CI (Node.js CIruns on every PR intogallagher, including future integration-merge PRs) with a banner explaining exactly what happened and the one-line fix to reapply — rather than regressing silently.Changes
lib/plugins/teams.js— normalizename→sluginfind().test/unit/lib/plugins/teams.slug-fix.test.js(new) — asserts (1) an unchanged team with mismatched display name/slug produces no nop output and no API calls, and (2) a genuine permission change still reports cleanly as a modification only, without phantom additions/deletions.Test plan
npx jest --roots=lib --roots=test/unit— full unit suite passesnpx eslint/npx standardon changed files — cleanlib/plugins/teams.js(viagit stash) to simulate an upstream integration merge overwriting it, confirmedteams.slug-fix.test.jsfails with a clear "reapply this patch" message, then restored and confirmed it passes againNext step after this merges: submit this same fix as a PR to upstream (
github-community-projects/safe-settings), since it addresses their own issue github-community-projects#1033 directly.