fix: full sync all app installations, not just the first

[rafaelleonardocruz]

Problem

syncInstallation() paginates apps.listInstallations but then operates on installations[0] only:

https://github.com/github-community-projects/safe-settings/blob/main-enterprise/index.js#L234-L246

Both full-sync entry points go through it:

• the scheduled CRON sync (index.js, cron.schedule(process.env.CRON, () => { syncInstallation() }))
• the standalone runner (full-sync.js, npm run full-sync)

So when a single safe-settings App is installed on more than one organization, only one org is ever swept. The others are silently skipped — no error, no log line. They keep whatever drift they have until a webhook happens to fire for them, which means the periodic safety net that full-sync exists to provide simply does not apply to them.

We hit this running one App installed on two organizations: the hourly full sync only ever reconciled the first installation, and the second org — the one where we most needed a periodic backstop — was covered only by real-time webhook events.

Change

syncInstallation() now iterates every installation, authenticating per installation and building the same context as before (admin repo scoped to that installation's account.login) before calling syncAllSettings.

Three details worth reviewing:

1. Per-installation isolation. Each iteration is wrapped in try/catch, so one broken installation (suspended, revoked permissions, missing admin repo) cannot abort the sync of the remaining ones — that would defeat the purpose of a safety net. The failure is logged with the installation id and account login, then collected.

2. Aggregate return { results, errors }. results holds the successful per-installation return values in order; errors concatenates every result.errors plus one entry per failed iteration. This keeps the full-sync.js contract working — it inspects settings.errors and exits non-zero when non-empty — and now it exits non-zero if any installation failed rather than only the first. null is still returned when there are no installations, unchanged.

3. One info log line. The CRON tick logs at debug and syncInstallation at trace, so at the default LOG_LEVEL=info a scheduled full sync is completely invisible: you cannot tell from the logs whether it ran. A single summary line (Synced N of M installation(s); F failed) at the end makes the hourly run observable without adding noise. Per-installation detail stays at debug.

info() is deliberately not changed — its use of installations[0] is legitimate, since the app slug it resolves is a property of the App rather than of any one installation.

Tests

New test/unit/sync-installation.test.js (8 tests), driving the exported plugin with a fake robot and the injectable Settings argument:

• two installations → syncAll called once per installation, each with its own owner
• the nop flag is passed through to every installation
• errors reported by individual syncs are aggregated
• an installation that throws → the remaining installations are still synced, the failure surfaces in errors, and the id/account are logged
• zero installations → returns null, nothing synced
• the summary line is logged exactly once

Reverting index.js to its current state fails 6 of the 8, so the suite pins the new behaviour rather than merely passing alongside it.

npx jest test/unit/sync-installation.test.js   → 8 passed
npx jest --roots=lib --roots=test/unit         → 142 passed, 14 skipped (no regressions)
npx standard test/unit/sync-installation.test.js && npx eslint test/unit/sync-installation.test.js  → clean

One heads-up so it is not attributed to this PR: npx standard index.js / npx eslint index.js report index.js:5:7 'Glob' is assigned a value but never used. That is pre-existing on main-enterprise (verified by stashing this diff and re-running); removing the dead require felt like it belonged in a separate cleanup PR rather than here.

Notes / possible follow-ups

• Installations are synced sequentially, deliberately: it keeps the diff minimal and avoids secondary rate limits. A concurrency-capped fan-out would be a reasonable follow-up for installs numbering in the hundreds.
• With many installations a single CRON tick naturally takes longer, and each installation's work still has to fit inside its own installation-token lifetime. Sequential execution makes that easy to reason about, but operators of very large fleets may want to widen the CRON interval.

Happy to adjust the return shape (for example index-aligned results with holes for failures, or keeping the bare single-installation return when only one exists) if you would prefer a different contract.

---

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Fixes the full-sync path so it reconciles all GitHub App installations (across multiple orgs), not just the first installation returned by apps.listInstallations. This makes scheduled and manual full-sync runs act as a true safety net for every org where the app is installed.

Changes:

• Update syncInstallation() to iterate through every installation, authenticating and syncing per-installation, isolating failures, and aggregating { results, errors }.
• Add a single info-level summary log line after the sweep to make CRON full-sync runs observable at default log levels.
• Add a focused unit test suite covering multi-installation fan-out, error aggregation, failure isolation, and logging behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
index.js	Iterates all installations in syncInstallation(), aggregates results/errors, and logs a summary line.
test/unit/sync-installation.test.js	Adds unit tests to pin the new multi-installation full-sync behavior and logging.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.