From fd2d26e67213e280700fde3e4e983c7ad049a65f Mon Sep 17 00:00:00 2001 From: Yadhav Jayaraman <57544838+decyjphr@users.noreply.github.com> Date: Mon, 28 Sep 2026 23:25:21 -0400 Subject: [PATCH 1/2] fix: target full sync with GH_ORG Adapt PR #1053 source 020145722fb1251f53d7331ccba6bdbc56217a60 onto yadhav/fix-recent-issues at 188c604929201774699fd1d8dabd445358cee9e8. Preserve the first-installation default and enterprise context, and return scheduled sync promises so targeting failures are reported. Cover case-insensitive selection, fail-closed targeting, NOP and CLI failures, scheduler reporting, and explicit-org smoke dry runs. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .env.example | 3 + README.md | 19 ++ docs/github-action.md | 6 + index.js | 15 +- lib/env.js | 1 + smoke-test.js | 46 ++- test/unit/lib/env.test.js | 62 ++--- .../unit/smoke-organization-targeting.test.js | 88 ++++++ test/unit/sync-installation.test.js | 261 ++++++++++++++++++ 9 files changed, 464 insertions(+), 37 deletions(-) create mode 100644 test/unit/smoke-organization-targeting.test.js create mode 100644 test/unit/sync-installation.test.js diff --git a/.env.example b/.env.example index d98f7d4b9..c8292167f 100644 --- a/.env.example +++ b/.env.example @@ -1,6 +1,9 @@ # The organization where you want to register the app in the app creation manifest flow. # If set, the app is registered for an organization (https://github.com/organizations/ORGANIZATION/settings/apps/new), # if not set, the GitHub app would be registered for the user account (https://github.com/settings/apps/new). +# Also selects the account for CLI and cron full syncs (case-insensitive). +# If set without a matching installation, the sync fails instead of falling back. +# If unset, full sync uses the first installation returned by the API. # GH_ORG= # The ID of your GitHub App diff --git a/README.md b/README.md index 052d21e99..436ba0a87 100644 --- a/README.md +++ b/README.md @@ -951,6 +951,21 @@ You can pass environment variables; the easiest way to do it is via a `.env` fil ``` CREATE_DEFAULT_BRANCH=true ``` +1. Scope CLI and cron full syncs to one account using `GH_ORG`. For e.g. + ``` + GH_ORG=my-org + ``` + A full sync (`CRON` or `npm run full-sync`) selects the installation whose + account login matches `GH_ORG` case-insensitively and reads configuration from + that account's ``. If no installation matches, including when the + app has no installations, the sync fails instead of syncing another account. + This also applies to dry runs (`FULL_SYNC_NOP=true`); the CLI exits nonzero on + a targeting error, and cron reports the execution as failed. + When `GH_ORG` is unset, the first installation returned by the API is used, + so set it whenever the app is installed on more than one account. + Webhook events still use their own installation context. `GH_ORG` is also used + by the [manifest flow](https://docs.github.com/en/apps/sharing-github-apps/registering-a-github-app-from-a-manifest) + to choose where the app is registered. ### Runtime Settings @@ -1037,6 +1052,9 @@ node smoke-test.js --phase 1,3,5 # Bypass actor apply/NOP convergence (Phase 1 creates the required test repo) node smoke-test.js --phase 1,19 +# Explicit GH_ORG full-sync CLI dry run (setup + Phase 24 + teardown) +node smoke-test.js --phase 24 + # Mix range + interactive npm run smoke-test:phase -- 1-3 interactive node smoke-test.js --phase 1-3 --interactive @@ -1063,6 +1081,7 @@ The smoke test runs the following phases: | **Phase 12** | Tests `custom_properties` plugin | | **Phase 13** | Tests the `variables` plugin (create, update, remove variables) | | **Phase 19** | Tests ignored `OrganizationAdmin`/`DeployKey` IDs, order-independent NOP convergence, real bypass-mode/role-ID changes, and no redundant updates (requires Phase 1) | +| **Phase 24** | Runs the real full-sync CLI with explicit `GH_ORG` and `FULL_SYNC_NOP=true`, verifying the selected installation and successful completion; multi-account, no-match, and cron failure scenarios use local mocked tests | | **Teardown** | Shuts down safe-settings, deletes test repos, teams, custom roles, and rulesets | ### Output diff --git a/docs/github-action.md b/docs/github-action.md index 10c284497..e084ef5ff 100644 --- a/docs/github-action.md +++ b/docs/github-action.md @@ -22,6 +22,12 @@ for suborg configuration changes; existing repository restrictions still apply. ### Example GHA Workflow The below example uses the GHA "cron" feature to run a full-sync every 4 hours. While not required, this example uses the `.github` repo as the `admin` repo (set via `ADMIN_REPO` env var) and the safe-settings configurations are stored in the `safe-settings/` directory (set via `CONFIG_PATH` and `DEPLOYMENT_CONFIG_FILE`). +`GH_ORG` selects the account to sync, matching its login case-insensitively. +Set it when the App is installed on multiple accounts; without it, full sync +uses the first installation returned by the API. If no installation matches, +the sync exits nonzero rather than syncing another account, including in dry +runs (`FULL_SYNC_NOP=true`). + ```yaml name: Safe Settings Sync on: diff --git a/index.js b/index.js index d4c588424..79a495a28 100644 --- a/index.js +++ b/index.js @@ -418,8 +418,17 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) => robot.log.trace('Fetching installations') const installations = await listAllInstallations() - if (installations.length > 0) { - const installation = installations[0] + const installation = env.GH_ORG + ? installations.find(i => i.account?.login?.toLowerCase() === env.GH_ORG.toLowerCase()) + : installations[0] + + if (env.GH_ORG && !installation) { + const accounts = installations.map(i => i.account?.login).join(', ') + throw new Error(`No app installation found for GH_ORG '${env.GH_ORG}'. Installed on: [${accounts}]`) + } + + if (installation) { + robot.log.info(`Syncing installation ${installation.id} on account ${installation.account?.login}`) const github = await robot.auth(installation.id) const context = { payload: { @@ -1017,7 +1026,7 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) => */ cron.schedule(process.env.CRON, () => { robot.log.debug('running a task every minute') - syncInstallation() + return syncInstallation() }) } diff --git a/lib/env.js b/lib/env.js index 7cdc467c5..252e976b4 100644 --- a/lib/env.js +++ b/lib/env.js @@ -8,6 +8,7 @@ module.exports = { BLOCK_REPO_RENAME_BY_HUMAN: process.env.BLOCK_REPO_RENAME_BY_HUMAN || 'false', CREATE_DEFAULT_BRANCH: process.env.CREATE_DEFAULT_BRANCH === 'true', FULL_SYNC_NOP: process.env.FULL_SYNC_NOP === 'true', + GH_ORG: process.env.GH_ORG, GHE_HOST: process.env.GHE_HOST, GHE_PROTOCOL: process.env.GHE_PROTOCOL, } diff --git a/smoke-test.js b/smoke-test.js index 96688fcfc..b53876df3 100644 --- a/smoke-test.js +++ b/smoke-test.js @@ -148,6 +148,7 @@ class InteractiveExit extends Error { // ─── Octokit client (initialized in main) ──────────────────────────────────── let octokit = null +let orgInstallation = null // Enterprise-installation-authenticated client (Phase 17). Null when the app // is not installed on the enterprise or GH_ENTERPRISE is unset. let entOctokit = null @@ -3447,6 +3448,47 @@ async function phase19BypassActorConvergence () { } } +async function phase24OrganizationSyncTargeting () { + logPhase('Phase 24: Organization-targeted full-sync dry run') + if (!assert(process.env.GH_ORG && orgInstallation?.account?.login?.toLowerCase() === ORG.toLowerCase(), + '24: explicit GH_ORG matches the authenticated test installation')) { + throw new Error('Phase 24 requires an explicitly configured and verified test organization') + } + + const result = await new Promise((resolve, reject) => { + const child = spawn(process.execPath, [path.join(__dirname, 'full-sync.js')], { + cwd: __dirname, + env: { + ...process.env, + GH_ORG: ORG, + FULL_SYNC_NOP: 'true', + CRON: '', + LOG_LEVEL: 'info' + }, + stdio: ['ignore', 'pipe', 'pipe'], + timeout: 120000 + }) + let output = '' + child.stdout.on('data', data => { output += data }) + child.stderr.on('data', data => { output += data }) + child.on('error', error => { + logFail(`24: could not start full-sync CLI: ${error.message}`) + reject(error) + }) + child.on('close', (code, signal) => resolve({ code, signal, output })) + }) + + const selected = [...result.output.matchAll(/Syncing installation (\d+) on account ([A-Za-z0-9-]+)/g)] + assert(result.code === 0, `24: full-sync CLI exited successfully (code=${result.code}, signal=${result.signal})`) + assert(result.output.includes('Starting full sync with NOP=true'), '24: full-sync CLI ran in NOP mode') + assert(selected.length === 1 && Number(selected[0][1]) === orgInstallation.id && + selected[0][2].toLowerCase() === ORG.toLowerCase(), + '24: sync selected exactly the authenticated test organization installation') + assert(result.output.includes('Full sync completed successfully.'), '24: actual full-sync entrypoint completed') + assert(!/Unexpected error during full sync|Fatal error during full sync|Errors occurred during full sync/.test(result.output), + '24: full-sync CLI reported no failure') +} + async function main () { const { App } = await import('octokit') const app = new App({ appId: APP_ID, privateKey: PRIVATE_KEY }) @@ -3457,6 +3499,7 @@ async function main () { // Org/user installations key off account.login (only enterprise accounts have a slug). if (installation.account && installation.account.login && installation.account.login.toLowerCase() === ORG.toLowerCase()) { installationId = installation.id + orgInstallation = installation break } } @@ -3517,7 +3560,8 @@ async function main () { ['Phase 17: App installation management', phase17AppInstallations], ['Phase 18: Team include/exclude filters', phase18TeamIncludeExclude], ['Phase 19: Bypass actor convergence', phase19BypassActorConvergence], - ['Phase 20: Custom property exclusions', phase20CustomPropertyExclusions] + ['Phase 20: Custom property exclusions', phase20CustomPropertyExclusions], + ['Phase 24: Organization sync targeting', phase24OrganizationSyncTargeting] ] // When --phase is given, only run setup (phase 0) + the requested phase(s). diff --git a/test/unit/lib/env.test.js b/test/unit/lib/env.test.js index d345fe543..7daec359d 100644 --- a/test/unit/lib/env.test.js +++ b/test/unit/lib/env.test.js @@ -1,48 +1,43 @@ /* eslint-disable no-undef */ describe('env', () => { - describe('load default values without override', () => { - const envTest = require('../../../lib/env') - - it('loads default ADMIN_REPO if not passed', () => { - const ADMIN_REPO = envTest.ADMIN_REPO - expect(ADMIN_REPO).toEqual('admin') - }) - - it('loads default CONFIG_PATH if not passed', () => { - const CONFIG_PATH = envTest.CONFIG_PATH - expect(CONFIG_PATH).toEqual('.github') - }) - - it('loads default SETTINGS_FILE_PATH if not passed', () => { - const SETTINGS_FILE_PATH = envTest.SETTINGS_FILE_PATH - expect(SETTINGS_FILE_PATH).toEqual('settings.yml') - }) - - it('loads default DEPLOYMENT_CONFIG_FILE_PATH if not passed', () => { - const SETTINGS_FILE_PATH = envTest.DEPLOYMENT_CONFIG_FILE_PATH - expect(SETTINGS_FILE_PATH).toEqual('deployment-settings.yml') - }) + const originalEnv = process.env + const defaults = { + ADMIN_REPO: 'admin', + CONFIG_PATH: '.github', + SETTINGS_FILE_PATH: 'settings.yml', + DEPLOYMENT_CONFIG_FILE_PATH: 'deployment-settings.yml', + CREATE_PR_COMMENT: 'true', + FULL_SYNC_NOP: false, + GH_ORG: undefined + } + + beforeEach(() => { + jest.resetModules() + process.env = { ...originalEnv } + for (const key of [...Object.keys(defaults), 'DEPLOYMENT_CONFIG_FILE']) { + delete process.env[key] + } + }) - it('loads default CREATE_PR_COMMENT if not passed', () => { - const CREATE_PR_COMMENT = envTest.CREATE_PR_COMMENT - expect(CREATE_PR_COMMENT).toEqual('true') - }) + afterEach(() => { + process.env = originalEnv + }) - it('loads default FULL_SYNC_NOP if not passed', () => { - const FULL_SYNC_NOP = envTest.FULL_SYNC_NOP - expect(FULL_SYNC_NOP).toEqual(false) + describe('load default values without override', () => { + it.each(Object.entries(defaults))('loads default %s if not passed', (key, value) => { + expect(require('../../../lib/env')[key]).toEqual(value) }) }) describe('load override values', () => { - beforeAll(() => { - jest.resetModules() + beforeEach(() => { process.env.ADMIN_REPO = '.github' process.env.CONFIG_PATH = '.config' process.env.SETTINGS_FILE_PATH = 'safe-settings.yml' process.env.DEPLOYMENT_CONFIG_FILE = 'safe-settings-deployment.yml' process.env.CREATE_PR_COMMENT = 'false' - process.env.FULL_SYNC_NOP = false + process.env.FULL_SYNC_NOP = 'true' + process.env.GH_ORG = 'My-Org' }) it('loads override values if passed', () => { @@ -58,7 +53,8 @@ describe('env', () => { const CREATE_PR_COMMENT = envTest.CREATE_PR_COMMENT expect(CREATE_PR_COMMENT).toEqual('false') const FULL_SYNC_NOP = envTest.FULL_SYNC_NOP - expect(FULL_SYNC_NOP).toEqual(false) + expect(FULL_SYNC_NOP).toEqual(true) + expect(envTest.GH_ORG).toEqual('My-Org') }) }) }) diff --git a/test/unit/smoke-organization-targeting.test.js b/test/unit/smoke-organization-targeting.test.js new file mode 100644 index 000000000..079b421c2 --- /dev/null +++ b/test/unit/smoke-organization-targeting.test.js @@ -0,0 +1,88 @@ +/* eslint-disable no-undef */ +const fs = require('fs') +const path = require('path') +const vm = require('vm') +const { EventEmitter } = require('events') + +const smokePath = path.resolve(__dirname, '../../smoke-test.js') +const source = fs.readFileSync(smokePath, 'utf8') +const phaseSource = source.slice( + source.indexOf('async function phase24OrganizationSyncTargeting ()'), + source.indexOf('async function main ()') +) + +describe('organization-targeted smoke phase', () => { + let child, spawn, check, logFail, run, env + const success = 'Starting full sync with NOP=true\nSyncing installation 2 on account My-Org\nFull sync completed successfully.' + + beforeEach(() => { + child = new EventEmitter() + child.stdout = new EventEmitter() + child.stderr = new EventEmitter() + spawn = jest.fn(() => child) + check = jest.fn(condition => Boolean(condition)) + logFail = jest.fn() + env = { GH_ORG: 'my-org', FULL_SYNC_NOP: 'false', CRON: '* * * * *' } + // Execute only this phase, never the live harness setup or its .env loader. + run = vm.runInNewContext(`(${phaseSource})`, { + spawn, + assert: check, + logPhase: jest.fn(), + logFail, + process: { execPath: process.execPath, env }, + path, + __dirname: path.dirname(smokePath), + ORG: 'my-org', + orgInstallation: { id: 2, account: { login: 'My-Org' } } + }) + }) + + it('runs only the scoped CLI dry run with no background cron', async () => { + const pending = run() + child.stdout.emit('data', success) + child.emit('close', 0, null) + await pending + + expect(spawn).toHaveBeenCalledWith(process.execPath, [path.resolve(__dirname, '../../full-sync.js')], expect.objectContaining({ + timeout: 120000, + env: expect.objectContaining({ GH_ORG: 'my-org', FULL_SYNC_NOP: 'true', CRON: '', LOG_LEVEL: 'info' }) + })) + expect(check).toHaveBeenCalledTimes(6) + expect(check.mock.results.every(result => result.value)).toBe(true) + expect(env.FULL_SYNC_NOP).toBe('false') + expect(env.CRON).toBe('* * * * *') + }) + + it.each([ + ['wrong account', success.replace('My-Org', 'other-org'), 0], + ['wrong installation', success.replace('installation 2', 'installation 1'), 0], + ['extra installation', success + '\nSyncing installation 1 on account other-org', 0], + ['apply instead of NOP', success.replace('NOP=true', 'NOP=false'), 0], + ['incomplete execution', success.replace('Full sync completed successfully.', ''), 0], + ['CLI failure', success, 1], + ['timeout', success, null], + ['fatal output', success + '\nUnexpected error during full sync: failure', 0] + ])('fails its assertions for %s', async (_name, output, code) => { + const pending = run() + child.stdout.emit('data', output) + child.emit('close', code, code === null ? 'SIGTERM' : null) + await pending + + expect(check.mock.results.some(result => !result.value)).toBe(true) + }) + + it('refuses to run with an implicit test organization', async () => { + delete env.GH_ORG + + await expect(run()).rejects.toThrow('explicitly configured and verified test organization') + expect(spawn).not.toHaveBeenCalled() + }) + + it('reports process startup errors as failures', async () => { + const pending = run() + child.emit('error', new Error('spawn failed')) + + await expect(pending).rejects.toThrow('spawn failed') + expect(logFail).toHaveBeenCalledWith('24: could not start full-sync CLI: spawn failed') + }) +}) diff --git a/test/unit/sync-installation.test.js b/test/unit/sync-installation.test.js new file mode 100644 index 000000000..8f345fad8 --- /dev/null +++ b/test/unit/sync-installation.test.js @@ -0,0 +1,261 @@ +/* eslint-disable no-undef */ +const path = require('path') +const { spawnSync } = require('child_process') + +jest.mock('node-cron', () => ({ schedule: jest.fn() })) + +function installation (id, login) { + return { id, account: { login } } +} + +function client () { + return { + rest: { + apps: { + getAuthenticated: jest.fn().mockResolvedValue({ data: { slug: 'safe-settings' } }) + }, + repos: { + getContent: jest.fn().mockResolvedValue({ data: { content: '' } }) + } + } + } +} + +describe('syncInstallation', () => { + const originalEnv = process.env + let robot, appClient, clients, syncAll, handleError + + beforeEach(() => { + jest.resetModules() + process.env = { ...originalEnv } + for (const key of ['GH_ORG', 'GH_ENTERPRISE', 'CRON', 'ADMIN_REPO', 'CONFIG_PATH', 'SETTINGS_FILE_PATH']) { + delete process.env[key] + } + process.env.DEPLOYMENT_CONFIG_FILE = path.join(__dirname, 'no-such-deployment-settings.yml') + appClient = { + paginate: jest.fn().mockResolvedValue([]), + rest: { + apps: { + listInstallations: { endpoint: { merge: jest.fn(options => options) } } + } + } + } + clients = new Map([[1, client()], [2, client()], [3, client()]]) + robot = { + auth: jest.fn(async id => { + if (id === undefined) return appClient + if (!clients.has(id)) throw new Error(`Unexpected installation ${id}`) + return clients.get(id) + }), + log: { debug: jest.fn(), info: jest.fn(), warn: jest.fn(), error: jest.fn(), trace: jest.fn() }, + on: jest.fn() + } + syncAll = jest.fn().mockResolvedValue({ errors: [] }) + handleError = jest.fn() + }) + + afterEach(() => { + process.env = originalEnv + }) + + async function load (ghOrg, installations) { + if (ghOrg !== undefined) process.env.GH_ORG = ghOrg + appClient.paginate.mockResolvedValue(installations) + const app = require('../../index')(robot, {}, { syncAll, handleError }) + // info() intentionally authenticates independently to read the app slug. + await new Promise(resolve => setImmediate(resolve)) + robot.auth.mockClear() + appClient.paginate.mockClear() + return app + } + + function expectSync (selected, nop = false) { + const repo = { owner: selected.account.login, repo: process.env.ADMIN_REPO || 'admin' } + expect(syncAll).toHaveBeenCalledTimes(1) + expect(syncAll).toHaveBeenCalledWith(nop, expect.objectContaining({ + payload: { installation: selected }, + octokit: clients.get(selected.id), + log: robot.log + }), repo, expect.any(Object)) + expect(syncAll.mock.calls[0][1].repo()).toEqual(repo) + expect(clients.get(selected.id).rest.repos.getContent).toHaveBeenCalledWith(expect.objectContaining(repo)) + expect(appClient.rest.apps.listInstallations.endpoint.merge).toHaveBeenCalledWith({ per_page: 100 }) + } + + it.each(['my-org', 'MY-ORG', 'My-Org'])('selects only the matching installation for GH_ORG=%s', async ghOrg => { + const selected = installation(2, 'my-org') + process.env.ADMIN_REPO = 'config' + const app = await load(ghOrg, [installation(1, 'other-org'), selected]) + + await app.syncInstallation() + + expectSync(selected) + expect(robot.auth.mock.calls).toEqual([[], [2]]) + expect(clients.get(1).rest.repos.getContent).not.toHaveBeenCalled() + expect(clients.get(1).rest.apps.getAuthenticated).toHaveBeenCalledTimes(1) + expect(robot.log.info).toHaveBeenCalledWith('Syncing installation 2 on account my-org') + }) + + it.each([false, true])('rejects an absent target without authenticating or syncing another installation (nop=%s)', async nop => { + const app = await load('my-org', [installation(1, 'other-org'), installation(2, 'another-org')]) + + await expect(app.syncInstallation(nop)).rejects.toThrow( + "No app installation found for GH_ORG 'my-org'. Installed on: [other-org, another-org]" + ) + + expect(robot.auth.mock.calls).toEqual([[]]) + expect(syncAll).not.toHaveBeenCalled() + expect(handleError).not.toHaveBeenCalled() + }) + + it.each([false, true])('rejects an empty installation list with a configured target (nop=%s)', async nop => { + const app = await load('my-org', []) + + await expect(app.syncInstallation(nop)).rejects.toThrow( + "No app installation found for GH_ORG 'my-org'. Installed on: []" + ) + expect(robot.auth.mock.calls).toEqual([[]]) + expect(syncAll).not.toHaveBeenCalled() + }) + + it.each([undefined, ''])('preserves the first-installation default with GH_ORG=%s', async ghOrg => { + const selected = installation(1, 'first-org') + const app = await load(ghOrg, [selected, installation(2, 'second-org')]) + + await app.syncInstallation() + + expectSync(selected) + expect(robot.auth.mock.calls).toEqual([[], [1]]) + expect(clients.get(2).rest.repos.getContent).not.toHaveBeenCalled() + }) + + it('still returns null when there are no installations and no configured org', async () => { + const app = await load(undefined, []) + + await expect(app.syncInstallation()).resolves.toBeNull() + expect(syncAll).not.toHaveBeenCalled() + }) + + it('passes the NOP flag and returns the original sync result', async () => { + const selected = installation(2, 'my-org') + const result = { errors: [{ msg: 'existing sync error' }] } + syncAll.mockResolvedValue(result) + const app = await load('my-org', [installation(1, 'other-org'), selected]) + + await expect(app.syncInstallation(true)).resolves.toBe(result) + expectSync(selected, true) + }) + + it('preserves enterprise enrichment and selects an org rather than an enterprise account', async () => { + const selected = { ...installation(2, 'my-org'), enterprise: { slug: 'My-Enterprise' } } + const enterprise = { id: 3, target_type: 'Enterprise', account: { slug: 'my-enterprise' } } + const app = await load('my-org', [installation(1, 'other-org'), enterprise, selected]) + + await app.syncInstallation() + + expectSync(selected) + expect(syncAll.mock.calls[0][1]).toMatchObject({ + enterpriseSlug: 'My-Enterprise', + appGithub: clients.get(3) + }) + expect(robot.auth.mock.calls).toEqual([[], [2], [], [3]]) + expect(clients.get(1).rest.repos.getContent).not.toHaveBeenCalled() + expect(clients.get(3).rest.repos.getContent).not.toHaveBeenCalled() + }) + + it.each(['listing', 'authentication', 'config', 'sync'])('preserves original %s failures', async stage => { + const error = new Error(`${stage} failed`) + const app = await load('my-org', [installation(2, 'my-org')]) + if (stage === 'listing') appClient.paginate.mockRejectedValue(error) + if (stage === 'authentication') { + robot.auth.mockImplementation(async id => { + if (id !== undefined) throw error + return appClient + }) + } + if (stage === 'config') clients.get(2).rest.repos.getContent.mockRejectedValue(error) + if (stage === 'sync') syncAll.mockRejectedValue(error) + + await expect(app.syncInstallation()).rejects.toBe(error) + if (stage !== 'sync') expect(syncAll).not.toHaveBeenCalled() + }) + + it('returns the scheduled promise so node-cron reports targeting failures instead of success', async () => { + process.env.CRON = '* * * * *' + await load('my-org', [installation(1, 'other-org')]) + const schedule = require('node-cron').schedule + expect(schedule).toHaveBeenCalledWith('* * * * *', expect.any(Function)) + + await expect(schedule.mock.calls[0][1]()).rejects.toThrow("No app installation found for GH_ORG 'my-org'") + expect(syncAll).not.toHaveBeenCalled() + }) + + it('awaits successful scheduled syncs', async () => { + process.env.CRON = '* * * * *' + await load('my-org', [installation(2, 'my-org')]) + const result = { errors: [] } + syncAll.mockResolvedValue(result) + + await expect(require('node-cron').schedule.mock.calls[0][1]()).resolves.toBe(result) + }) + + it('reports a failed execution through the real node-cron task lifecycle', async () => { + process.env.CRON = '* * * * *' + await load('my-org', [installation(1, 'other-org')]) + const callback = require('node-cron').schedule.mock.calls[0][1] + const task = jest.requireActual('node-cron').createTask('* * * * *', callback) + const failed = jest.fn() + const finished = jest.fn() + task.on('execution:failed', failed) + task.on('execution:finished', finished) + const logged = jest.spyOn(console, 'error').mockImplementation(() => {}) + + try { + await expect(task.execute()).rejects.toThrow("No app installation found for GH_ORG 'my-org'") + expect(failed).toHaveBeenCalledTimes(1) + expect(finished).not.toHaveBeenCalled() + expect(logged).toHaveBeenCalled() + } finally { + task.destroy() + } + }) + + it.each([false, true])('exits the real full-sync CLI nonzero on targeting failure (nop=%s)', nop => { + const cli = spawnSync(process.execPath, ['-e', ` + const fs = require('fs') + const vm = require('vm') + const plugin = require(process.argv[1]) + const github = { + paginate: async () => [{ id: 1, account: { login: 'other-org' } }], + rest: { apps: { + listInstallations: { endpoint: { merge: options => options } }, + getAuthenticated: async () => ({ data: { slug: 'safe-settings' } }) + } } + } + const robot = { auth: async () => github, on: () => {}, log: console } + vm.runInNewContext(fs.readFileSync(process.argv[2], 'utf8'), { + require: name => { + if (name === './') return (robot, options) => plugin(robot, options, { + syncAll: () => { throw new Error('Wrong account synced') } + }) + if (name === './lib/env') return { FULL_SYNC_NOP: process.env.FULL_SYNC_NOP === 'true' } + if (name === 'probot') return { createProbot: () => robot } + throw new Error('Unexpected dependency: ' + name) + }, + process, + console + }) + `, require.resolve('../../index'), require.resolve('../../full-sync')], { + encoding: 'utf8', + env: { ...process.env, GH_ORG: 'my-org', FULL_SYNC_NOP: String(nop) }, + timeout: 10000 + }) + + expect(cli.error).toBeUndefined() + expect(cli.status).toBe(1) + expect(cli.stdout).toContain(`Starting full sync with NOP=${nop}`) + expect(cli.stdout).toContain("No app installation found for GH_ORG 'my-org'") + expect(cli.stdout).not.toContain('Wrong account synced') + expect(cli.stdout).not.toContain('Full sync completed successfully.') + }) +}) From a408d67184e44aed656303a71f11122f12e552e8 Mon Sep 17 00:00:00 2001 From: Yadhav Jayaraman <57544838+decyjphr@users.noreply.github.com> Date: Tue, 29 Sep 2026 00:00:51 -0400 Subject: [PATCH 2/2] fix: await Probot readiness before full sync Wait for the installed Probot runtime to initialize before using its logger or loading the application. Preserve existing full-sync reporting and exit behavior. Add no-network real-runtime CLI regressions for apply and dry-run success, missing targets, and collected sync failures; update existing CLI mock fixtures. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- full-sync.js | 2 +- test/unit/full-sync.test.js | 102 ++++++++++++++++++++++++ test/unit/lib/settings-batching.test.js | 2 +- test/unit/sync-installation.test.js | 2 +- 4 files changed, 105 insertions(+), 3 deletions(-) create mode 100644 test/unit/full-sync.test.js diff --git a/full-sync.js b/full-sync.js index 8ba1c7353..267996c49 100644 --- a/full-sync.js +++ b/full-sync.js @@ -3,7 +3,7 @@ const { FULL_SYNC_NOP } = require('./lib/env') const { createProbot } = require('probot') async function performFullSync (appFn, nop) { - const probot = createProbot() + const probot = await createProbot().ready() probot.log.info(`Starting full sync with NOP=${nop}`) try { diff --git a/test/unit/full-sync.test.js b/test/unit/full-sync.test.js new file mode 100644 index 000000000..b2082bf3f --- /dev/null +++ b/test/unit/full-sync.test.js @@ -0,0 +1,102 @@ +/* eslint-disable no-undef */ +const { spawnSync } = require('child_process') +const { generateKeyPairSync } = require('crypto') +const path = require('path') + +describe('full-sync CLI with the installed Probot runtime', () => { + let privateKey + + beforeAll(() => { + privateKey = generateKeyPairSync('rsa', { + modulusLength: 2048, + privateKeyEncoding: { type: 'pkcs1', format: 'pem' }, + publicKeyEncoding: { type: 'pkcs1', format: 'pem' } + }).privateKey + }) + + it.each([ + [false, 'success'], + [true, 'success'], + [false, 'missing-target'], + [true, 'missing-target'], + [false, 'sync-errors'], + [true, 'sync-errors'] + ])('initializes before logging and preserves NOP=%s %s reporting', (nop, scenario) => { + const cli = spawnSync(process.execPath, ['-e', ` + const forbidNetwork = () => { throw new Error('Network access forbidden in CLI regression') } + global.fetch = forbidNetwork + require('http').request = forbidNetwork + require('https').request = forbidNetwork + require('net').Socket.prototype.connect = forbidNetwork + + const { Probot } = require('probot') + Probot.prototype.auth = async function (id) { + await this.ready() + if (id !== undefined && ![1, 2].includes(id)) throw new Error('Unexpected installation') + return { + paginate: async () => [ + { id: 1, account: { login: 'other-org' } }, + { id: 2, account: { login: 'test-org' } } + ], + rest: { + apps: { + listInstallations: { endpoint: { merge: options => options } }, + getAuthenticated: async () => ({ data: { slug: 'test-app' } }) + }, + repos: { getContent: async () => ({ data: { content: '' } }) } + } + } + } + const Module = require('module') + const load = Module._load + Module._load = function (name, parent, main) { + if (name === './lib/settings' && parent.filename === require.resolve('./index')) { + return { + syncAll: async (nop, context, repo) => { + console.log('SYNC_RESULT ' + JSON.stringify({ nop, owner: repo.owner, installation: context.payload.installation.id })) + return { errors: process.env.TEST_SCENARIO === 'sync-errors' ? [{ msg: 'test sync failure' }] : [] } + } + } + } + return load.call(this, name, parent, main) + } + require('./full-sync') + `], { + cwd: path.resolve(__dirname, '../..'), + encoding: 'utf8', + timeout: 10000, + env: { + PATH: process.env.PATH, + APP_ID: '1', + PRIVATE_KEY: privateKey, + GH_ORG: scenario === 'missing-target' ? 'missing-org' : 'TeSt-OrG', + FULL_SYNC_NOP: String(nop), + TEST_SCENARIO: scenario, + LOG_FORMAT: 'json', + LOG_LEVEL: 'info', + DEPLOYMENT_CONFIG_FILE: path.join(__dirname, 'no-such-deployment-settings.yml') + } + }) + const output = cli.stdout + cli.stderr + + expect(cli.error).toBeUndefined() + expect(output).not.toContain('Network access forbidden') + expect(output).toContain(`Starting full sync with NOP=${nop}`) + expect(cli.status).toBe(scenario === 'success' ? 0 : 1) + if (scenario === 'missing-target') { + expect(output).toContain("No app installation found for GH_ORG 'missing-org'") + expect(output).not.toContain('SYNC_RESULT') + } else { + expect(output).toContain('SYNC_RESULT ' + JSON.stringify({ nop, owner: 'test-org', installation: 2 })) + expect(output).toContain('Syncing installation 2 on account test-org') + } + if (scenario === 'success') { + expect(output).toContain('Full sync completed successfully.') + } else { + expect(output).not.toContain('Full sync completed successfully.') + } + if (scenario === 'sync-errors') { + expect(output).toContain('Errors occurred during full sync.') + } + }) +}) diff --git a/test/unit/lib/settings-batching.test.js b/test/unit/lib/settings-batching.test.js index d97f47855..9c78fb3ce 100644 --- a/test/unit/lib/settings-batching.test.js +++ b/test/unit/lib/settings-batching.test.js @@ -327,7 +327,7 @@ describe('Repository sync batching', () => { require: name => { if (name === './') return () => ({ syncInstallation: async () => settings }) if (name === './lib/env') return { FULL_SYNC_NOP: true } - if (name === 'probot') return { createProbot: () => ({ log: console }) } + if (name === 'probot') return { createProbot: () => ({ ready: async () => ({ log: console }) }) } throw new Error('Unexpected dependency: ' + name) }, process, diff --git a/test/unit/sync-installation.test.js b/test/unit/sync-installation.test.js index 8f345fad8..35edebba2 100644 --- a/test/unit/sync-installation.test.js +++ b/test/unit/sync-installation.test.js @@ -232,7 +232,7 @@ describe('syncInstallation', () => { getAuthenticated: async () => ({ data: { slug: 'safe-settings' } }) } } } - const robot = { auth: async () => github, on: () => {}, log: console } + const robot = { auth: async () => github, on: () => {}, log: console, ready: async () => robot } vm.runInNewContext(fs.readFileSync(process.argv[2], 'utf8'), { require: name => { if (name === './') return (robot, options) => plugin(robot, options, {