Address app installation review: remove bad drift handler, fix ordering, docs

- Remove installation.repositories_added/removed handler: an app only
  receives those events for its own installation, so they cannot detect
  drift on managed apps. Drift is reconciled by the scheduled full sync.
- Process repository_unselection before repository_selection (delta and full
  sync) so a repo removed by one config and added by another ends up present
- Add test asserting removal-before-addition ordering
- Document app_installations in README (prerequisites, hierarchy, examples,
  sync behavior, drift note, disable/additive support) and add it to the
  configurable-items and disable_plugins lists

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: decyjphr

README.md

@@ -552,7 +552,8 @@ plugins for a given scope. Each entry is either:
 
 Valid plugin names: `repository`, `labels`, `collaborators`, `teams`,
 `milestones`, `branches`, `autolinks`, `validator`, `rulesets`, `environments`,
-`custom_properties`, `custom_repository_roles`, `variables`, `archive`.
+`custom_properties`, `custom_repository_roles`, `variables`, `archive`,
+`app_installations`.
 
 #### Strip matrix (which source layers are removed before merge)
 
@@ -661,6 +662,105 @@ additive_plugins:
   - collaborators
 ```
 
+### App installation management (`app_installations`)
+
+Most safe-settings plugins target a **repository**. The `app_installations`
+plugin is different: its target is a **GitHub App installation**. It lets you
+declaratively manage *which repositories a GitHub App can access* (the app's
+`repository_selection`), using the same `org` → `suborg` → `repo` config
+hierarchy you already use for repository settings.
+
+This is useful for controlling, as code, which repos apps such as Copilot,
+Dependabot, or your own internal apps are installed on across the org.
+
+#### Prerequisites
+
+- Safe-settings must be installed on the **enterprise** with the **Enterprise
+  organization installations** permission (see the
+  [Enterprise organization installations API](https://docs.github.com/en/enterprise-cloud@latest/rest/enterprise-admin/organization-installations)).
+  Managing app installations requires an enterprise-level token; the regular
+  org installation token is not sufficient. If safe-settings is not installed
+  on the enterprise with this permission, app installation sync is reported as
+  an error and skipped.
+- The enterprise slug is read from the webhook event payload
+  (`payload.enterprise.slug`); no extra environment variable is required.
+
+#### How repository selection is resolved
+
+The config layer where `app_installations` is declared determines which repos
+are selected for the app:
+
+| Layer | File | Repos selected for the app |
+| --- | --- | --- |
+| Org | `settings.yml` | All repos in the org (`repository_selection: all`) |
+| Suborg | `suborgs/*.yml` | Repos matching the suborg's targeting (`suborgrepos`, `suborgteams`, `suborgproperties`) |
+| Repo | `repos/<repo>.yml` | That specific repo |
+
+> [!important]
+> An app configured with `repository_selection: all` at the **org** level takes
+> precedence. Suborg/repo-level selections for that same app are ignored, and
+> repos are never removed from it by incremental (suborg/repo) changes — it is
+> reconciled only by the full (scheduled) sync.
+
+#### Examples
+
+Org-level `settings.yml` — give an app access to **all** repos in the org:
+
+```yaml
+app_installations:
+  - app_slug: my-internal-app
+    repository_selection: all
+```
+
+Suborg-level `suborgs/backend.yml` — give an app access to the repos targeted
+by this suborg (here, all repos with the `Team=backend` custom property):
+
+```yaml
+suborgproperties:
+  - Team: backend
+app_installations:
+  - app_slug: my-internal-app
+```
+
+Repo-level `repos/my-repo.yml` — add this specific repo to the app:
+
+```yaml
+app_installations:
+  - app_slug: my-internal-app
+```
+
+Removing an app from a suborg/repo config (or changing the suborg's targeting)
+removes the affected repos from that app on the next sync, unless another layer
+still selects them.
+
+#### Sync behavior
+
+- **Incremental (delta) sync** runs when a `suborgs/*.yml` or `repos/*.yml`
+  file changes. Only the apps affected by the changed file are reconciled: the
+  previous version of the file is compared with the new one to compute repos to
+  add (`repository_selection`) and repos to remove (`repository_unselection`).
+  Removals are applied before additions, so a repo removed by one config and
+  added by another ends up present.
+- **Full sync** runs on the schedule (cron), on manual sync, and when
+  `settings.yml` changes. It recomputes the full desired state for every managed
+  app across all layers and reconciles it against the live installation state.
+  This is the mechanism that corrects any configuration drift.
+- Add/remove operations are automatically batched in chunks of 50 repos (the
+  API limit).
+
+> [!note]
+> Drift on managed apps is reconciled by the **full (cron) sync**, not by
+> webhooks. A GitHub App only receives `installation` repository events for its
+> *own* installation, so safe-settings cannot detect — via webhooks — when a
+> human changes another app's repository access. Keep the scheduled sync enabled
+> for timely drift correction.
+
+#### Disabling and additive mode
+
+`app_installations` honors both [`disable_plugins`](#disabling-plugins-disable_plugins)
+and [`additive_plugins`](#additive-plugins-additive_plugins). In additive mode
+the plugin only **adds** repos to installations and never removes them.
+
 ### The Settings Files
 
 The settings files can be used to set the policies at the `org`, `suborg` or `repo` level.
@@ -680,6 +780,7 @@ The following can be configured:
 - `Repository name validation` using regex pattern
 - `Rulesets`
 - `Environments` - wait timer, required reviewers, prevent self review, protected branches deployment branch policy, custom deployment branch policy, variables, deployment protection rules
+- `App installations` - which repositories a GitHub App installation can access (see [App installation management](#app-installation-management-app_installations))
 
 See [`docs/sample-settings/settings.yml`](docs/sample-settings/settings.yml) for a sample settings file.
 

index.js

@@ -531,32 +531,16 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) =>
   })
 
   // ────────────────────────────────────────────────────────────────────────
-  // App installation drift detection handlers
+  // App installation target handler
+  //
+  // Note: We intentionally do NOT handle `installation.repositories_added` /
+  // `installation.repositories_removed`. A GitHub App only receives those
+  // events for its OWN installation, not for the managed apps (e.g. Copilot,
+  // Dependabot) whose repository access safe-settings controls. They cannot
+  // detect drift on managed apps, so drift is reconciled by the scheduled
+  // (cron) full sync instead.
   // ────────────────────────────────────────────────────────────────────────
 
-  const installation_change_events = [
-    'installation.repositories_added',
-    'installation.repositories_removed'
-  ]
-
-  robot.on(installation_change_events, async context => {
-    const { payload } = context
-    const { sender } = payload
-    robot.log.debug('App installation repos changed by ', JSON.stringify(sender))
-    if (sender.type === 'Bot') {
-      robot.log.debug('App installation repos changed by Bot')
-      return
-    }
-    robot.log.debug('App installation repos changed by a Human — triggering sync to revert drift')
-
-    // Build a context that targets the admin repo for this org
-    const orgLogin = payload.installation.account.login
-    const updatedContext = Object.assign({}, context, {
-      repo: () => { return { repo: env.ADMIN_REPO, owner: orgLogin } }
-    })
-    return syncAllSettings(false, updatedContext)
-  })
-
   robot.on('installation_target', async context => {
     const { payload } = context
     const { sender } = payload

lib/plugins/appInstallations.js

@@ -149,18 +149,20 @@ class AppInstallations {
           continue
         }
 
-        // Resolve names to IDs for additions (need to look up IDs)
-        if (toAdd.size > 0) {
-          const addIds = await this._resolveRepoIds([...toAdd])
-          await this.enterpriseClient.addReposToInstallation(desired.installation_id, addIds)
-          this.log.debug(`App '${appSlug}': added ${addIds.length} repos`)
-        }
-
+        // Resolve names to IDs. Process removals first, then additions, so a
+        // repo that should be both removed (by one config) and added (by
+        // another) ends up present.
         if (toRemove.size > 0) {
           const removeIds = [...toRemove].map(name => liveRepoMap.get(name)).filter(Boolean)
           await this.enterpriseClient.removeReposFromInstallation(desired.installation_id, removeIds)
           this.log.debug(`App '${appSlug}': removed ${removeIds.length} repos`)
         }
+
+        if (toAdd.size > 0) {
+          const addIds = await this._resolveRepoIds([...toAdd])
+          await this.enterpriseClient.addReposToInstallation(desired.installation_id, addIds)
+          this.log.debug(`App '${appSlug}': added ${addIds.length} repos`)
+        }
       } catch (e) {
         this.log.error(`Error in full sync for app '${appSlug}': ${e.message}`)
         this.errors.push({
@@ -254,18 +256,18 @@ class AppInstallations {
       return results
     }
 
-    if (hasSelections) {
-      const addIds = await this._resolveRepoIds([...repository_selection])
-      await this.enterpriseClient.addReposToInstallation(installation_id, addIds)
-      this.log.debug(`App '${app_slug}': added ${addIds.length} repos`)
-    }
-
     if (hasUnselections) {
       const removeIds = await this._resolveRepoIds([...repository_unselection])
       await this.enterpriseClient.removeReposFromInstallation(installation_id, removeIds)
       this.log.debug(`App '${app_slug}': removed ${removeIds.length} repos`)
     }
 
+    if (hasSelections) {
+      const addIds = await this._resolveRepoIds([...repository_selection])
+      await this.enterpriseClient.addReposToInstallation(installation_id, addIds)
+      this.log.debug(`App '${app_slug}': added ${addIds.length} repos`)
+    }
+
     return results
   }
 

test/unit/lib/plugins/appInstallations.test.js

@@ -107,26 +107,32 @@ describe('AppInstallations', () => {
       expect(result[0].action.deletions).toBeNull()
     })
 
-    it('adds repos via enterprise client in non-nop mode', async () => {
-      github.repos.get
-        .mockResolvedValueOnce({ data: { id: 100 } })
-        .mockResolvedValueOnce({ data: { id: 200 } })
+    it('processes unselections before selections in non-nop mode', async () => {
+      // repo-a (add) resolves to id 100; repo-b (remove) resolves to id 200
+      github.repos.get.mockImplementation(({ repo }) => {
+        if (repo === 'repo-a') return Promise.resolve({ data: { id: 100 } })
+        if (repo === 'repo-b') return Promise.resolve({ data: { id: 200 } })
+        return Promise.resolve({ data: { id: 0 } })
+      })
+
+      const callOrder = []
+      appGithub.request.mockImplementation((route) => {
+        if (route.startsWith('DELETE')) callOrder.push('remove')
+        if (route.startsWith('POST')) callOrder.push('add')
+        return Promise.resolve({ data: {} })
+      })
 
       const plugin = new AppInstallations(false, github, appGithub, { owner: 'org', repo: 'admin' }, 'ent', log, errors)
       await plugin.syncDelta([{
         app_slug: 'copilot',
         installation_id: 1,
-        repository_selection: new Set(['repo-a', 'repo-b']),
-        repository_unselection: new Set()
+        repository_selection: new Set(['repo-a']),
+        repository_unselection: new Set(['repo-b'])
       }])
 
-      // Should have called request to add repos
-      expect(appGithub.request).toHaveBeenCalledWith(
-        expect.stringContaining('POST'),
-        expect.objectContaining({
-          repository_ids: [100, 200]
-        })
-      )
+      // Removal must be applied before addition so a repo removed by one
+      // config and added by another ends up present.
+      expect(callOrder).toEqual(['remove', 'add'])
     })
   })