test(config): cover error propagation and live config loading

README.md

@@ -1091,9 +1091,17 @@ The smoke test runs the following phases:
 | **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 21** | Reads all pages of 101 repository variables and 100 environment variables, verifies unchanged NOP/apply writes nothing, and updates only the two boundary variables (requires Phases 1 and 13) |
 | **Phase 22** | Runs real full-sync NOP against the verified `GH_ORG` installation with controlled enumeration/auth and read-only, org-scoped requests. Requires only Setup; multi-installation fanout and failure isolation are covered by unit/CLI tests, not this single-org smoke |
+| **Phase 23** | Tests real config loading, empty YAML, directories, and missing-file/ref HTTP 404s on an owned temporary branch (requires only Setup) |
 | **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 |
 
+Run config-loading and existing config-validation coverage with
+`node smoke-test.js --phase 10,23` (Setup and Teardown run automatically).
+Phase 23 removes its temporary `smoke-test-phase23` branch even on failure.
+HTTP 403/500 and network failures are injected only in local unit tests, which
+also verify original-error identity and full-sync/NOP reporting; the live smoke
+does not change permissions or provoke rate limits.
+
 The harness rejects a nonempty `CRON` before authentication or setup. However,
 the spawned Probot CLI reloads `.env` and can override explicit environment values.
 This check does not guarantee that the server's CRON, webhook forwarding, or

smoke-test.js

@@ -3540,6 +3540,47 @@ async function phase19BypassActorConvergence () {
   }
 }
 
+async function phase23ConfigLoading () {
+  logPhase('Phase 23: Config loading')
+  const ConfigManager = require('./lib/configManager')
+  const env = require('./lib/env')
+  const branch = 'smoke-test-phase23'
+  const filePath = path.posix.join(CONFIG_PATH, env.SETTINGS_FILE_PATH)
+  const context = {
+    repo: () => ({ owner: ORG, repo: ADMIN_REPO }),
+    octokit,
+    log: { error: log, debug: log, info: log }
+  }
+  const manager = new ConfigManager(context, branch)
+  let created = false
+  try {
+    // Creating rather than replacing the ref protects any pre-existing branch.
+    await createBranch(ORG, ADMIN_REPO, branch)
+    created = true
+    await createOrUpdateFile(ORG, ADMIN_REPO, filePath, 'repository:\n  has_wiki: false\n', branch, '23: config loading fixture')
+    assert(JSON.stringify(await manager.loadGlobalSettingsYaml()) === JSON.stringify({ repository: { has_wiki: false } }),
+      '23: real global config is parsed from the requested branch')
+    await createOrUpdateFile(ORG, ADMIN_REPO, filePath, '# empty\n', branch, '23: empty config fixture')
+    assert(JSON.stringify(await manager.loadGlobalSettingsYaml()) === '{}', '23: empty YAML returns an empty object')
+    assert(await manager.loadYaml(CONFIG_PATH) === null, '23: real directory response returns null')
+    assert(await manager.loadYaml(path.posix.join(CONFIG_PATH, 'smoke-missing-config-23.yml')) === null,
+      '23: missing config file returns null on real HTTP 404')
+    assert(await new ConfigManager(context, `${branch}-missing`).loadGlobalSettingsYaml() === null,
+      '23: missing ref returns null on real HTTP 404')
+  } catch (error) {
+    logFail(`23: config loading failed: ${error.message}`)
+    throw error
+  } finally {
+    if (created) {
+      try {
+        await deleteBranch(ORG, ADMIN_REPO, branch)
+      } catch (error) {
+        logFail(`23: could not remove fixture branch: ${error.message}`)
+      }
+    }
+  }
+}
+
 async function phase24OrganizationSyncTargeting () {
   logPhase('Phase 24: Organization-targeted full-sync dry run')
   if (!assert(process.env.GH_ORG && orgInstallation?.account?.login?.toLowerCase() === ORG.toLowerCase(),
@@ -3735,6 +3776,7 @@ async function main () {
       ['Phase 20: Custom property exclusions', phase20CustomPropertyExclusions],
       ['Phase 21: Variable pagination', phase21VariablePagination],
       ['Phase 22: Test-org installation full-sync NOP', () => phase22InstallationFullSync(app, installationId)],
+      ['Phase 23: Config loading', phase23ConfigLoading],
       ['Phase 24: Organization sync targeting', phase24OrganizationSyncTargeting]
     ]
 

test/unit/index-config-errors.test.js

@@ -0,0 +1,116 @@
+const fs = require('fs')
+const plugin = require('../../index')
+const Settings = require('../../lib/settings')
+const env = require('../../lib/env')
+
+describe('configuration read error reporting', () => {
+  let app
+  let handlers
+  let github
+  let context
+
+  beforeEach(() => {
+    jest.replaceProperty(process, 'env', { ...process.env, CRON: '', GH_ENTERPRISE: '' })
+    jest.replaceProperty(env, 'ADMIN_REPO', 'admin')
+    jest.replaceProperty(env, 'CONFIG_PATH', '.github')
+    jest.replaceProperty(env, 'SETTINGS_FILE_PATH', 'settings.yml')
+    jest.replaceProperty(env, 'CREATE_PR_COMMENT', 'true')
+    jest.replaceProperty(env, 'GH_ORG', 'test-org')
+    jest.spyOn(fs, 'existsSync').mockReturnValue(false)
+    handlers = new Map()
+    const installation = { id: 123, account: { login: 'test-org' } }
+    github = {
+      paginate: jest.fn().mockResolvedValue([installation]),
+      rest: {
+        apps: {
+          listInstallations: { endpoint: { merge: jest.fn().mockReturnValue({}) } },
+          getAuthenticated: jest.fn().mockResolvedValue({ data: { slug: 'safe-settings' } })
+        },
+        repos: { getContent: jest.fn() },
+        pulls: { listFiles: jest.fn().mockResolvedValue({ data: [{ filename: Settings.FILE_PATH }] }) },
+        checks: { update: jest.fn().mockResolvedValue({}) },
+        issues: { createComment: jest.fn().mockResolvedValue({}) }
+      }
+    }
+    const log = { debug: jest.fn(), info: jest.fn(), error: jest.fn(), trace: jest.fn() }
+    const robot = {
+      auth: jest.fn().mockResolvedValue(github),
+      log,
+      on: (events, handler) => {
+        for (const event of [].concat(events)) handlers.set(event, handler)
+      }
+    }
+    context = {
+      repo: () => ({ owner: 'test-org', repo: 'admin' }),
+      octokit: github,
+      log,
+      payload: {
+        installation,
+        repository: { name: 'admin', default_branch: 'main', owner: { login: 'test-org' } },
+        check_run: {
+          id: 42,
+          name: 'Safe-setting validator',
+          status: 'queued',
+          check_suite: {
+            pull_requests: [{ number: 1, head: { ref: 'config-change' }, base: { ref: 'main' } }]
+          }
+        }
+      }
+    }
+    app = plugin(robot, {}, Settings)
+  })
+
+  const errors = [
+    Object.assign(new Error('Forbidden'), { status: 403 }),
+    Object.assign(new Error('Internal Server Error'), { status: 500 }),
+    Object.assign(new Error('Connection reset'), { code: 'ECONNRESET' })
+  ]
+
+  it.each(errors)('preserves %s in the full-sync error aggregate', async error => {
+    github.rest.repos.getContent.mockRejectedValue(error)
+
+    const result = await app.syncInstallation(false)
+
+    expect(result.results).toEqual([])
+    expect(result.errors).toHaveLength(1)
+    expect(result.errors[0]).toBe(error)
+    expect(github.rest.repos.getContent).toHaveBeenCalledWith({
+      owner: 'test-org',
+      repo: 'admin',
+      path: '.github/settings.yml',
+      ref: undefined
+    })
+    expect(github.rest.checks.update).not.toHaveBeenCalled()
+    expect(github.rest.issues.createComment).not.toHaveBeenCalled()
+  })
+
+  it.each(errors)('reports %s in the failed NOP check and PR comment', async error => {
+    github.rest.repos.getContent.mockRejectedValue(error)
+    const handleError = jest.spyOn(Settings, 'handleError')
+
+    await handlers.get('check_run.created')(context)
+    expect(handleError).toHaveBeenCalledTimes(1)
+    const command = handleError.mock.calls[0][5]
+    expect(command.type).toBe('ERROR')
+    expect(command.plugin).toBe('settings.yml')
+    expect(command.action).toBe(error)
+    await handleError.mock.results[0].value
+
+    expect(github.rest.repos.getContent).toHaveBeenCalledWith({
+      owner: 'test-org',
+      repo: 'admin',
+      path: '.github/settings.yml',
+      ref: 'config-change'
+    })
+    expect(github.rest.checks.update).toHaveBeenCalledTimes(2)
+    const check = github.rest.checks.update.mock.calls[1][0]
+    expect(check).toMatchObject({ check_run_id: 42, status: 'completed', conclusion: 'failure' })
+    expect(github.rest.issues.createComment).toHaveBeenCalledTimes(1)
+    const comment = github.rest.issues.createComment.mock.calls[0][0]
+    expect(comment).toMatchObject({ owner: 'test-org', repo: 'admin', issue_number: 1 })
+    for (const output of [check.output.summary, comment.body]) {
+      expect(output).toContain(error.message)
+      expect(output).not.toContain('TypeError')
+    }
+  })
+})

test/unit/lib/configManager.test.js

@@ -0,0 +1,174 @@
+/* eslint-disable no-undef */
+const ConfigManager = require('../../../lib/configManager')
+const env = require('../../../lib/env')
+const { YAMLException } = require('js-yaml')
+
+describe('configManager', () => {
+  let context
+
+  beforeEach(() => {
+    jest.replaceProperty(env, 'ADMIN_REPO', 'admin')
+    jest.replaceProperty(env, 'CONFIG_PATH', '.github')
+    jest.replaceProperty(env, 'SETTINGS_FILE_PATH', 'settings.yml')
+    context = {
+      repo: () => { return { owner: 'test-org', repo: 'event-repo' } },
+      octokit: {
+        rest: {
+          repos: {
+            getContent: jest.fn()
+          }
+        }
+      },
+      log: {
+        debug: jest.fn(),
+        info: jest.fn(),
+        error: jest.fn()
+      }
+    }
+  })
+
+  describe('loadYaml', () => {
+    it('returns the parsed YAML content when the file is fetched successfully', async () => {
+      const configManager = new ConfigManager(context, 'main')
+      context.octokit.rest.repos.getContent.mockResolvedValue({
+        data: { content: Buffer.from('key: value').toString('base64') }
+      })
+
+      const result = await configManager.loadYaml('.github/settings.yml')
+
+      expect(result).toEqual({ key: 'value' })
+      expect(context.octokit.rest.repos.getContent).toHaveBeenCalledWith({
+        owner: 'test-org',
+        repo: 'admin',
+        path: '.github/settings.yml',
+        ref: 'main'
+      })
+    })
+
+    it('returns null when the path is a folder', async () => {
+      const configManager = new ConfigManager(context, 'main')
+      context.octokit.rest.repos.getContent.mockResolvedValue({ data: [] })
+
+      await expect(configManager.loadYaml('.github')).resolves.toBeNull()
+    })
+
+    it.each([
+      { type: 'symlink', target: 'settings.yml' },
+      { type: 'submodule', submodule_git_url: 'https://github.com/test-org/config.git' },
+      { content: null }
+    ])('returns undefined for a response without string content: %j', async data => {
+      const configManager = new ConfigManager(context, 'main')
+      context.octokit.rest.repos.getContent.mockResolvedValue({ data })
+
+      await expect(configManager.loadYaml('.github/settings.yml')).resolves.toBeUndefined()
+    })
+
+    it.each(['', '# empty\n', 'null\n'])('preserves empty YAML semantics for %j', async content => {
+      const configManager = new ConfigManager(context, 'main')
+      context.octokit.rest.repos.getContent.mockResolvedValue({
+        data: { content: Buffer.from(content).toString('base64') }
+      })
+
+      await expect(configManager.loadYaml('.github/settings.yml')).resolves.toEqual({})
+    })
+
+    it('propagates YAML parsing errors without replacing them with a TypeError', async () => {
+      const configManager = new ConfigManager(context, 'main')
+      context.octokit.rest.repos.getContent.mockResolvedValue({
+        data: { content: Buffer.from('repository: [').toString('base64') }
+      })
+
+      await expect(configManager.loadYaml('.github/settings.yml')).rejects.toBeInstanceOf(YAMLException)
+    })
+
+    it('returns null when the file does not exist', async () => {
+      const configManager = new ConfigManager(context, 'main')
+      const notFound = new Error('Not Found')
+      notFound.status = 404
+      context.octokit.rest.repos.getContent.mockRejectedValue(notFound)
+
+      await expect(configManager.loadYaml('.github/settings.yml')).resolves.toBeNull()
+    })
+
+    it('propagates a non-404 error instead of masking it', async () => {
+      const configManager = new ConfigManager(context, 'main')
+      const serverError = new Error('Internal Server Error')
+      serverError.status = 500
+      context.octokit.rest.repos.getContent.mockRejectedValue(serverError)
+
+      await expect(configManager.loadYaml('.github/settings.yml')).rejects.toThrow('Internal Server Error')
+      await expect(configManager.loadYaml('.github/settings.yml')).rejects.toBe(serverError)
+    })
+
+    it('propagates the original error object so its status is preserved', async () => {
+      const configManager = new ConfigManager(context, 'main')
+      const forbidden = new Error('Forbidden')
+      forbidden.status = 403
+      context.octokit.rest.repos.getContent.mockRejectedValue(forbidden)
+
+      await expect(configManager.loadYaml('.github/settings.yml')).rejects.toBe(forbidden)
+    })
+
+    it('preserves a network error without an HTTP status', async () => {
+      const configManager = new ConfigManager(context)
+      const networkError = Object.assign(new Error('Connection reset'), { code: 'ECONNRESET' })
+      context.octokit.rest.repos.getContent.mockRejectedValue(networkError)
+
+      await expect(configManager.loadYaml('.github/settings.yml')).rejects.toBe(networkError)
+    })
+  })
+
+  describe('loadGlobalSettingsYaml', () => {
+    it('loads the settings file from the configured config path', async () => {
+      const configManager = new ConfigManager(context, 'main')
+      context.octokit.rest.repos.getContent.mockResolvedValue({
+        data: { content: Buffer.from('repository:\n  has_wiki: false').toString('base64') }
+      })
+
+      const result = await configManager.loadGlobalSettingsYaml()
+
+      expect(result).toEqual({ repository: { has_wiki: false } })
+      expect(context.octokit.rest.repos.getContent).toHaveBeenCalledWith({
+        owner: 'test-org',
+        repo: 'admin',
+        path: '.github/settings.yml',
+        ref: 'main'
+      })
+    })
+
+    it.each(['feature/config', undefined])('uses the configured admin repo and POSIX path at ref %s', async ref => {
+      jest.replaceProperty(env, 'ADMIN_REPO', 'policy-repo')
+      jest.replaceProperty(env, 'CONFIG_PATH', 'policies/nested/')
+      jest.replaceProperty(env, 'SETTINGS_FILE_PATH', 'org/settings.yaml')
+      const configManager = new ConfigManager(context, ref)
+      context.octokit.rest.repos.getContent.mockResolvedValue({
+        data: { content: Buffer.from('repository:\n  has_wiki: false').toString('base64') }
+      })
+
+      await expect(configManager.loadGlobalSettingsYaml()).resolves.toEqual({ repository: { has_wiki: false } })
+      expect(context.octokit.rest.repos.getContent).toHaveBeenCalledWith({
+        owner: 'test-org',
+        repo: 'policy-repo',
+        path: 'policies/nested/org/settings.yaml',
+        ref
+      })
+    })
+
+    it('returns null for a missing global settings file', async () => {
+      const configManager = new ConfigManager(context, 'main')
+      const error = Object.assign(new Error('Not Found'), { status: 404 })
+      context.octokit.rest.repos.getContent.mockRejectedValue(error)
+
+      await expect(configManager.loadGlobalSettingsYaml()).resolves.toBeNull()
+    })
+
+    it.each([403, 500, undefined])('preserves global settings read failures with status %s', async status => {
+      const configManager = new ConfigManager(context, 'main')
+      const error = new Error('Config read failed')
+      if (status !== undefined) error.status = status
+      context.octokit.rest.repos.getContent.mockRejectedValue(error)
+
+      await expect(configManager.loadGlobalSettingsYaml()).rejects.toBe(error)
+    })
+  })
+})