From 9b63a02b6ead5d472f657c9e8b2dabce043f7852 Mon Sep 17 00:00:00 2001 From: Rafa Leo Date: Mon, 3 Aug 2026 16:20:29 -0300 Subject: [PATCH 1/2] fix: full sync all app installations, not just the first `syncInstallation()` paginated `apps.listInstallations` but then only ever operated on `installations[0]`. When the App is installed on more than one organization, the scheduled CRON full sync and `npm run full-sync` swept a single org and silently skipped every other installation, leaving those orgs with no drift-correction safety net -- only webhook-driven correction. `syncInstallation()` now iterates every installation, authenticating per installation and building the same context (admin repo scoped to that installation's account login) before calling `syncAllSettings`. Each iteration is isolated in a try/catch so one broken installation (e.g. suspended, revoked permissions, missing admin repo) cannot abort the sync of the remaining ones. The failure is logged with the installation id and account login and collected instead of thrown. The return value is now an aggregate `{ results, errors }`: `results` holds the successful per-installation return values in order, and `errors` concatenates every `result.errors` plus one entry per failed iteration. This preserves the `full-sync.js` contract, which inspects `settings.errors` and exits non-zero when it is non-empty. `null` is still returned when there are no installations. Observability: the CRON tick logs at `debug` and `syncInstallation` at `trace`, so a scheduled sync was invisible at the default `LOG_LEVEL=info`. A single `info` summary line (synced / failed counts) is now emitted at the end, with per-installation detail kept at `debug`. `info()` is intentionally left alone: its use of `installations[0]` is correct, since the app slug it resolves is a property of the App, not of an installation. Co-Authored-By: Claude AI-Assisted: yes AI-Tool: claude-code Co-Authored-By: claude-code --- index.js | 48 +++++++--- test/unit/sync-installation.test.js | 141 ++++++++++++++++++++++++++++ 2 files changed, 177 insertions(+), 12 deletions(-) create mode 100644 test/unit/sync-installation.test.js diff --git a/index.js b/index.js index 12aae7c79..950e1529a 100644 --- a/index.js +++ b/index.js @@ -231,20 +231,44 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) => github.rest.apps.listInstallations.endpoint.merge({ per_page: 100 }) ) - if (installations.length > 0) { - const installation = installations[0] - const github = await robot.auth(installation.id) - const context = { - payload: { - installation - }, - octokit: github, - log: robot.log, - repo: () => { return { repo: env.ADMIN_REPO, owner: installation.account.login } } + if (installations.length === 0) { + return null + } + + // Sync every installation. A single failing installation must not prevent + // the remaining ones from being synced, so each iteration is isolated and + // its error is collected instead of thrown. + const results = [] + const errors = [] + let failed = 0 + + for (const installation of installations) { + try { + const owner = installation.account.login + robot.log.debug(`Syncing installation ${installation.id} for ${owner}`) + const github = await robot.auth(installation.id) + const context = { + payload: { + installation + }, + octokit: github, + log: robot.log, + repo: () => { return { repo: env.ADMIN_REPO, owner } } + } + const result = await syncAllSettings(nop, context) + results.push(result) + if (result?.errors?.length) { + errors.push(...result.errors) + } + } catch (e) { + failed++ + robot.log.error(`Failed to sync installation ${installation.id} for ${installation.account?.login}: ${e}`) + errors.push(e) } - return syncAllSettings(nop, context) } - return null + + robot.log.info(`Synced ${installations.length - failed} of ${installations.length} installation(s); ${failed} failed`) + return { results, errors } } robot.on('push', async context => { diff --git a/test/unit/sync-installation.test.js b/test/unit/sync-installation.test.js new file mode 100644 index 000000000..4e59bbac8 --- /dev/null +++ b/test/unit/sync-installation.test.js @@ -0,0 +1,141 @@ +const plugin = require('../../index') + +// The runtime config is fetched from the admin repo over the API. The +// installation fan-out under test does not depend on its contents. +jest.mock('../../lib/configManager', () => { + return class ConfigManager { + async loadGlobalSettingsYaml () { + return {} + } + } +}) + +describe('syncInstallation', () => { + let robot, Settings, installations + + const installation = (id, login) => ({ id, account: { login } }) + + const createOctokit = () => ({ + paginate: jest.fn(() => Promise.resolve(installations)), + rest: { + apps: { + listInstallations: { endpoint: { merge: jest.fn(() => ({})) } }, + getAuthenticated: jest.fn(() => Promise.resolve({ data: { slug: 'safe-settings' } })) + } + } + }) + + const createApp = () => plugin(robot, {}, Settings) + + beforeEach(() => { + installations = [] + Settings = { syncAll: jest.fn(() => Promise.resolve({ errors: [] })) } + robot = { + on: jest.fn(), + auth: jest.fn(() => Promise.resolve(createOctokit())), + log: { + trace: jest.fn(), + debug: jest.fn(), + info: jest.fn(), + error: jest.fn() + } + } + }) + + describe('with multiple installations', () => { + beforeEach(() => { + installations = [installation(1, 'org-one'), installation(2, 'org-two')] + }) + + it('syncs every installation with its own owner', async () => { + const app = createApp() + + const result = await app.syncInstallation() + + expect(Settings.syncAll).toHaveBeenCalledTimes(2) + expect(Settings.syncAll.mock.calls.map(call => call[2])).toEqual([ + { repo: 'admin', owner: 'org-one' }, + { repo: 'admin', owner: 'org-two' } + ]) + expect(result.results).toHaveLength(2) + expect(result.errors).toEqual([]) + }) + + it('passes the nop flag through to every installation', async () => { + const app = createApp() + + await app.syncInstallation(true) + + expect(Settings.syncAll.mock.calls.map(call => call[0])).toEqual([true, true]) + }) + + it('logs a single summary of the sync', async () => { + const app = createApp() + + await app.syncInstallation() + + expect(robot.log.info).toHaveBeenCalledTimes(1) + expect(robot.log.info).toHaveBeenCalledWith(expect.stringContaining('Synced 2 of 2 installation(s); 0 failed')) + }) + + it('aggregates errors reported by the individual syncs', async () => { + Settings.syncAll + .mockResolvedValueOnce({ errors: ['boom'] }) + .mockResolvedValueOnce({ errors: [] }) + const app = createApp() + + const result = await app.syncInstallation() + + expect(result.errors).toEqual(['boom']) + }) + }) + + describe('when an installation fails', () => { + const failure = new Error('installation is suspended') + + beforeEach(() => { + installations = [installation(1, 'org-one'), installation(2, 'org-two')] + Settings.syncAll + .mockRejectedValueOnce(failure) + .mockResolvedValueOnce({ errors: [] }) + }) + + it('still syncs the remaining installations', async () => { + const app = createApp() + + await app.syncInstallation() + + expect(Settings.syncAll).toHaveBeenCalledTimes(2) + expect(Settings.syncAll.mock.calls[1][2]).toEqual({ repo: 'admin', owner: 'org-two' }) + }) + + it('surfaces the failure in the returned errors', async () => { + const app = createApp() + + const result = await app.syncInstallation() + + expect(result.errors).toEqual([failure]) + expect(result.results).toHaveLength(1) + }) + + it('logs the failing installation id and account', async () => { + const app = createApp() + + await app.syncInstallation() + + expect(robot.log.error).toHaveBeenCalledWith(expect.stringContaining('installation 1 for org-one')) + expect(robot.log.info).toHaveBeenCalledWith(expect.stringContaining('Synced 1 of 2 installation(s); 1 failed')) + }) + }) + + describe('without any installation', () => { + it('returns null and does not sync', async () => { + const app = createApp() + + const result = await app.syncInstallation() + + expect(result).toBeNull() + expect(Settings.syncAll).not.toHaveBeenCalled() + }) + }) +}) From f720b1ab138aba942329b36e5385b1d48b5c843b Mon Sep 17 00:00:00 2001 From: Rafa Leo Date: Mon, 3 Aug 2026 16:55:16 -0300 Subject: [PATCH 2/2] fix: treat a missing sync result as an installation failure `syncAllSettings` rethrows in normal mode, but in nop mode its catch reports the problem through `Settings.handleError` and then falls through without returning anything. The per-installation loop pushed that `undefined` into `results` and counted the installation as synced, with nothing recorded in `errors`. The effect was that `npm run full-sync` with `FULL_SYNC_NOP=true` reported success for an installation whose configuration had failed to load, and `full-sync.js` exited 0. Before installations were iterated, the same case returned `undefined` from `syncInstallation`, so reading `settings.errors` in `full-sync.js` threw and the run exited non-zero. That was crude, but it was loud. For a drift-correction safety net, silently reporting a broken installation as healthy is worse than failing noisily. A falsy result is now treated as a failure of that installation: it is counted in the failed total, logged with its id and account login, and contributes an error to the aggregate, so a nop full sync still exits non-zero. Successful results keep their existing handling. `syncAllSettings` itself is deliberately left alone. `syncSettings` has the same shape and other callers depend on the current behavior, so changing the nop return value belongs in its own change. Co-Authored-By: Claude AI-Assisted: yes AI-Tool: claude-code Co-Authored-By: claude-code --- index.js | 12 ++++++++- test/unit/sync-installation.test.js | 40 +++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/index.js b/index.js index 950e1529a..3844f7379 100644 --- a/index.js +++ b/index.js @@ -256,8 +256,18 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) => repo: () => { return { repo: env.ADMIN_REPO, owner } } } const result = await syncAllSettings(nop, context) + if (!result) { + // In nop mode `syncAllSettings` reports the error and returns nothing. + // Counting that as a success would silently hide a broken config, so + // treat a missing result as a failure of this installation. + failed++ + const msg = `Sync of installation ${installation.id} for ${owner} returned no result` + robot.log.error(msg) + errors.push(new Error(msg)) + continue + } results.push(result) - if (result?.errors?.length) { + if (result.errors?.length) { errors.push(...result.errors) } } catch (e) { diff --git a/test/unit/sync-installation.test.js b/test/unit/sync-installation.test.js index 4e59bbac8..21720ba4a 100644 --- a/test/unit/sync-installation.test.js +++ b/test/unit/sync-installation.test.js @@ -128,6 +128,46 @@ describe('syncInstallation', () => { }) }) + describe('when a sync returns no result', () => { + // In nop mode `syncAllSettings` reports the error via `handleError` and + // returns nothing, which must not be mistaken for a successful sync. + beforeEach(() => { + installations = [installation(1, 'org-one'), installation(2, 'org-two')] + Settings.syncAll + .mockResolvedValueOnce(undefined) + .mockResolvedValueOnce({ errors: [] }) + }) + + it('counts the installation as failed and keeps it out of the results', async () => { + const app = createApp() + + const result = await app.syncInstallation(true) + + expect(result.results).toEqual([{ errors: [] }]) + expect(result.errors).toHaveLength(1) + expect(result.errors[0].message).toContain('installation 1 for org-one') + expect(result.errors[0].message).toContain('returned no result') + }) + + it('still syncs the remaining installations', async () => { + const app = createApp() + + await app.syncInstallation(true) + + expect(Settings.syncAll).toHaveBeenCalledTimes(2) + expect(Settings.syncAll.mock.calls[1][2]).toEqual({ repo: 'admin', owner: 'org-two' }) + }) + + it('reflects the failure in the summary log', async () => { + const app = createApp() + + await app.syncInstallation(true) + + expect(robot.log.error).toHaveBeenCalledWith(expect.stringContaining('installation 1 for org-one')) + expect(robot.log.info).toHaveBeenCalledWith(expect.stringContaining('Synced 1 of 2 installation(s); 1 failed')) + }) + }) + describe('without any installation', () => { it('returns null and does not sync', async () => { const app = createApp()