Skip to content

Fix: Patch comparison for Teams plugin to match on slug instead of name on NOP - #3

Open
StephHope wants to merge 1 commit into
gallagherfrom
fix-teams-slug-comparison
Open

Fix: Patch comparison for Teams plugin to match on slug instead of name on NOP#3
StephHope wants to merge 1 commit into
gallagherfrom
fix-teams-slug-comparison

Conversation

@StephHope

Copy link
Copy Markdown

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's name to its slug before the record reaches the shared diff engine (MergeDeep.compareDeep). GitHub's team API returns name as the display name (e.g. "Platform Engineering"), while config entries and the plugin's own comparator()/changed() treat name as the slug (e.g. platform-engineering). When those two didn't match textually, compareDeep failed 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.
  • The normalization is applied after checkSecurityManager(), since that filter matches on the real display name.

Why a separate test file
test/unit/lib/plugins/teams.slug-fix.test.js is intentionally its own file rather than an addition to teams.test.js. This keeps the regression isolated from upstream's own test file, so a future upstream integration merge onto gallagher won't silently clobber it, and if a merge ever does reintroduce the unpatched teams.js, this test fails loudly in CI (Node.js CI runs on every PR into gallagher, 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 — normalize nameslug in find().
  • 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 passes
  • npx eslint / npx standard on changed files — clean
  • Manually reverted lib/plugins/teams.js (via git stash) to simulate an upstream integration merge overwriting it, confirmed teams.slug-fix.test.js fails with a clear "reapply this patch" message, then restored and confirmed it passes again

Next 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.

@StephHope
StephHope requested a review from a team as a code owner August 3, 2026 02:50
@StephHope
StephHope requested a review from pvandervelde August 3, 2026 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants