Fix dry-run crash, check re-runs, cross-app runs and stuck checks

index.js

@@ -21,6 +21,8 @@ let deploymentConfig
 
 module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) => {
   let appSlug = 'safe-settings'
+  // Set by info() once the app has authenticated; undefined until then.
+  let appId
   // Cache of enterprise slug → enterprise installation id. Keyed by slug so a
   // cached id is never reused for a different enterprise (e.g. when the app
   // handles events from multiple enterprises).
@@ -362,6 +364,7 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) =>
       const github = await robot.auth(installation.id)
       const app = await github.rest.apps.getAuthenticated()
       appSlug = app.data.slug
+      appId = app.data.id
       robot.log.info(`Validated the app is configured properly = \n${JSON.stringify(app.data, null, 2)}`)
     }
 
@@ -731,7 +734,7 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) =>
     return syncAllSettings(false, updatedContext)
   })
 
-  robot.on('check_suite.requested', async context => {
+  robot.on(['check_suite.requested', 'check_suite.rerequested'], async context => {
     const { payload } = context
     const { repository } = payload
     const adminRepo = repository.name === env.ADMIN_REPO
@@ -769,7 +772,7 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) =>
       robot.log.debug('Not working on the Admin repo, returning...')
       return
     }
-    const defaultBranch = payload.pull_request.head_branch === repository.default_branch
+    const defaultBranch = payload.pull_request.head.ref === repository.default_branch
     if (defaultBranch) {
       robot.log.debug(' Working on the default branch, returning...')
       return
@@ -791,24 +794,14 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) =>
       return
     }
 
-    const defaultBranch = payload.pull_request.head_branch === repository.default_branch
+    const defaultBranch = payload.pull_request.head.ref === repository.default_branch
     if (defaultBranch) {
       robot.log.debug(' Working on the default branch, returning...')
       return
     }
     return createCheckRun(context, pull_request, payload.pull_request.head.sha, payload.pull_request.head.ref)
   })
 
-  robot.on(['check_suite.rerequested'], async context => {
-    robot.log.debug('Check suite was rerequested!')
-    return createCheckRun(context)
-  })
-
-  robot.on(['check_suite.rerequested'], async context => {
-    robot.log.debug('Check suite was rerequested!')
-    return createCheckRun(context)
-  })
-
   robot.on(['check_run.created'], async context => {
     robot.log.debug('Check run was created!')
     const { payload } = context
@@ -822,6 +815,13 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) =>
       return
     }
 
+    // Another safe-settings deployment (e.g. prod and non-prod apps on the same org)
+    // creates runs with the same name, but only the creating app can update them.
+    if (appId && check_run.app && check_run.app.id !== appId) {
+      robot.log.debug(' Check run belongs to another app, returning...')
+      return
+    }
+
     if (check_run.status === 'completed') {
       robot.log.debug(' Checkrun created as completed, returning')
       return
@@ -852,8 +852,9 @@ module.exports = (robot, { getRouter }, Settings = require('./lib/settings')) =>
 
     params = Object.assign(context.repo(), { pull_number: pull_request.number })
 
-    const changes = await context.octokit.rest.pulls.listFiles(params)
-    const files = changes.data.map(f => { return f.filename })
+    // listFiles returns at most 30 files per page (and 3,000 files in total), so paginate.
+    const changes = await context.octokit.paginate(context.octokit.rest.pulls.listFiles, { ...params, per_page: 100 })
+    const files = changes.map(f => { return f.filename })
 
     const settingsModified = files.includes(Settings.FILE_PATH)
     const repoChanges = getChangedRepoConfigName(files, context.repo().owner)

lib/plugins/repository.js

@@ -129,11 +129,12 @@ module.exports = class Repository extends ErrorStash {
         } else {
           promises.push(this.updateSecurity(resp.data, resArray))
         }
-        if (this.nop) {
-          return Promise.resolve(resArray)
-        } else {
-          return Promise.all(promises)
-        }
+        // Settle every side effect in both modes. In NOP mode they used to be left
+        // unawaited, so a failure (e.g. creating the default branch of an empty repo)
+        // became an unhandled rejection; in apply mode it fell through to the 404
+        // handler below and was mistaken for a missing repo.
+        const settled = Promise.all(promises).catch(e => this.handleSideEffectError(e, resArray))
+        return this.nop ? settled.then(() => resArray) : settled
       }).catch(e => {
         if (e.status === 404) {
           if (this.force_create) {
@@ -175,6 +176,14 @@ module.exports = class Repository extends ErrorStash {
       })
   }
 
+  handleSideEffectError (e, resArray) {
+    const msg = `Error applying repository settings for ${this.repo.owner}/${this.repo.repo}: ${e && e.message ? e.message : JSON.stringify(e)}`
+    if (this.nop) {
+      resArray.push(new NopCommand(this.constructor.name, this.repo, null, msg, 'ERROR'))
+    }
+    this.logError(msg)
+  }
+
   updateDefaultBranch (oldname, newname, resArray) {
     this.log.debug(`Checking if ${newname} is already a branch`)
     return this.github.rest.repos.getBranch({

lib/settings.js

@@ -1275,6 +1275,7 @@ class Settings {
 
     const renderedCommentMessage = await eta.renderString(commetMessageTemplate, stats)
 
+    let commentError
     if (env.CREATE_PR_COMMENT === 'true') {
       const pluginSectionList = renderChangeSections(stats.changeSections)
 
@@ -1357,14 +1358,20 @@ class Settings {
       const totalPages = pages.length
       const pullRequest = payload.check_run.check_suite.pull_requests[0]
 
-      for (let i = 0; i < pages.length; i++) {
-        const body = `${makeHeader(i + 1, totalPages)}${pages[i]}`
-        await this.github.rest.issues.createComment({
-          owner: payload.repository.owner.login,
-          repo: payload.repository.name,
-          issue_number: pullRequest.number,
-          body: truncateWithSuffix(body, COMMENT_LIMIT - footer.length, '... (too many changes to report)', Boolean(footer)) + footer
-        })
+      // A failed comment (size limit, 404, rate limit) must not leave the check run in_progress.
+      try {
+        for (let i = 0; i < pages.length; i++) {
+          const body = `${makeHeader(i + 1, totalPages)}${pages[i]}`
+          await this.github.rest.issues.createComment({
+            owner: payload.repository.owner.login,
+            repo: payload.repository.name,
+            issue_number: pullRequest.number,
+            body: truncateWithSuffix(body, COMMENT_LIMIT - footer.length, '... (too many changes to report)', Boolean(footer)) + footer
+          })
+        }
+      } catch (e) {
+        commentError = e
+        this.log.error(`Failed to comment on the PR with the dry-run results: ${e.message || JSON.stringify(e)}`)
       }
     }
 
@@ -1380,6 +1387,9 @@ class Settings {
         summary: truncateWithSuffix(renderedCommentMessage, COMMENT_LIMIT, '... (too many changes to report)')
       }
     }
+    if (commentError) {
+      params.output.text = `Posting the dry-run results as a PR comment failed: ${commentError.message || JSON.stringify(commentError)}`
+    }
 
     this.log.debug(`Completing check run ${JSON.stringify(params)}`)
     await this.github.rest.checks.update(params)

test/integration/transport/comment-markup.test.js

@@ -127,18 +127,29 @@ test('comments-disabled mode sends only a completed check', async () => {
   assert(output.summary.includes('`description`'))
 })
 
-for (const failure of ['comment', 'check']) {
-  test(`real transport propagates ${failure} failure instead of reporting success`, async () => {
-    const scope = nock(apiUrl).post('/repos/test-org/report-repo/issues/7/comments')
-    if (failure === 'comment') {
-      scope.reply(422, { message: 'Comment rejected' })
-    } else {
-      scope.reply(201, { id: 100 })
-        .patch('/repos/test-org/report-repo/check-runs/42').reply(422, { message: 'Check rejected' })
-    }
-    await assert.rejects((await reporter([change()])).handleResults(), error => error.status === 422)
-  })
-}
+test('real transport completes the check and reports the error when the comment fails', async () => {
+  let check
+  nock(apiUrl)
+    .post('/repos/test-org/report-repo/issues/7/comments').reply(422, { message: 'Comment rejected' })
+    .patch('/repos/test-org/report-repo/check-runs/42')
+    .reply((_uri, body) => { check = body; return [200, { id: 42, ...body }] })
+  const errors = []
+  const settings = await reporter([change()])
+  settings.log.error = message => errors.push(message)
+
+  await settings.handleResults()
+
+  assert.equal(check.status, 'completed')
+  assert.match(check.output.text, /Comment rejected/)
+  assert.equal(errors.length, 1)
+  assert.match(errors[0], /Failed to comment on the PR/)
+})
+
+test('real transport propagates check failure instead of reporting success', async () => {
+  nock(apiUrl).post('/repos/test-org/report-repo/issues/7/comments').reply(201, { id: 100 })
+    .patch('/repos/test-org/report-repo/check-runs/42').reply(422, { message: 'Check rejected' })
+  await assert.rejects((await reporter([change()])).handleResults(), error => error.status === 422)
+})
 
 function credentials (t) {
   const originalEnv = process.env

test/integration/transport/pr-comment-summary.test.js

@@ -141,7 +141,14 @@ for (const failAt of [
   test(`phase 27 preserves flags and removes only a successfully created fixture after ${failAt} fails`, async t => {
     const state = await fixture(t, { failAt })
     const flags = { ...env }
-    await assert.rejects(state.run, /Controlled reporting failure/)
+    const commentFails = failAt.endsWith('/comments')
+    // handleResults() completes the check when commenting fails, so the phase's own
+    // comment-count assertion is what stops the run.
+    await assert.rejects(state.run, commentFails ? /expected comment page count/ : /Controlled reporting failure/)
+    if (commentFails) {
+      assert(state.checks.length > 0)
+      assert(state.checks.every(check => check.status === 'completed'))
+    }
     assert.deepEqual(env, flags)
     assert.equal(state.deleted(), failAt !== 'POST /orgs/test-org/repos')
     assert(!state.logs.includes('Phase 27 complete'))

test/unit/index-check-runs.test.js

@@ -0,0 +1,173 @@
+const fs = require('fs')
+const plugin = require('../../index')
+const Settings = require('../../lib/settings')
+const env = require('../../lib/env')
+
+describe('validator check run handlers', () => {
+  let handlers
+  let github
+  let log
+
+  const repository = { name: 'admin', default_branch: 'main', owner: { login: 'test-org' } }
+
+  beforeEach(async () => {
+    jest.replaceProperty(process, 'env', { ...process.env, CRON: '', GH_ENTERPRISE: '' })
+    jest.replaceProperty(env, 'ADMIN_REPO', 'admin')
+    jest.spyOn(fs, 'existsSync').mockReturnValue(false)
+    handlers = new Map()
+    const installation = { id: 123, account: { login: 'test-org' } }
+    github = {
+      paginate: jest.fn(async (method, params) => {
+        if (method === github.rest.pulls.listFiles) return method(params)
+        return [installation]
+      }),
+      rest: {
+        apps: {
+          listInstallations: { endpoint: { merge: jest.fn().mockReturnValue({}) } },
+          getAuthenticated: jest.fn().mockResolvedValue({ data: { id: 1, slug: 'safe-settings' } })
+        },
+        repos: { getContent: jest.fn().mockResolvedValue({ data: { content: '' } }) },
+        pulls: { listFiles: jest.fn().mockResolvedValue([]) },
+        checks: {
+          create: jest.fn().mockResolvedValue({}),
+          update: jest.fn().mockResolvedValue({})
+        }
+      }
+    }
+    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)) {
+          if (!handlers.has(event)) handlers.set(event, [])
+          handlers.get(event).push(handler)
+        }
+      }
+    }
+    plugin(robot, {}, Settings)
+    // Let the startup info() call resolve the app's own identity.
+    await new Promise(resolve => setImmediate(resolve))
+  })
+
+  afterEach(() => {
+    jest.restoreAllMocks()
+  })
+
+  async function receive (event, payload) {
+    const context = {
+      payload,
+      octokit: github,
+      log,
+      repo: () => ({ owner: 'test-org', repo: payload.repository.name })
+    }
+    for (const handler of handlers.get(event) || []) {
+      await handler(context)
+    }
+  }
+
+  describe('check_suite.rerequested', () => {
+    function suite (overrides = {}) {
+      return {
+        repository,
+        check_suite: {
+          head_branch: 'config-change',
+          head_sha: 'abc123',
+          pull_requests: [{ number: 1 }],
+          ...overrides
+        }
+      }
+    }
+
+    it('creates one check run for the suite head_sha', async () => {
+      await receive('check_suite.rerequested', suite())
+
+      expect(github.rest.checks.create).toHaveBeenCalledTimes(1)
+      expect(github.rest.checks.create).toHaveBeenCalledWith({
+        owner: 'test-org',
+        repo: 'admin',
+        name: 'Safe-setting validator',
+        head_sha: 'abc123'
+      })
+    })
+
+    it('ignores suites outside the admin repo', async () => {
+      await receive('check_suite.rerequested', { ...suite(), repository: { ...repository, name: 'other' } })
+
+      expect(github.rest.checks.create).not.toHaveBeenCalled()
+    })
+
+    it('ignores suites without a PR', async () => {
+      await receive('check_suite.rerequested', suite({ pull_requests: [] }))
+
+      expect(github.rest.checks.create).not.toHaveBeenCalled()
+    })
+  })
+
+  describe.each(['pull_request.opened', 'pull_request.reopened'])('%s', event => {
+    function pr (ref) {
+      return { repository, pull_request: { number: 1, head: { ref, sha: 'abc123' } } }
+    }
+
+    it('creates a check run for a feature branch', async () => {
+      await receive(event, pr('config-change'))
+
+      expect(github.rest.checks.create).toHaveBeenCalledWith(expect.objectContaining({ head_sha: 'abc123' }))
+    })
+
+    // Behavior change: this guard used to read the non-existent pull_request.head_branch
+    // and never fired, so PRs from the default branch got a check run too.
+    it('skips PRs whose head is the default branch', async () => {
+      await receive(event, pr('main'))
+
+      expect(github.rest.checks.create).not.toHaveBeenCalled()
+    })
+  })
+
+  describe('check_run.created', () => {
+    function run (app) {
+      return {
+        repository,
+        check_run: {
+          id: 42,
+          name: 'Safe-setting validator',
+          status: 'queued',
+          app,
+          check_suite: {
+            pull_requests: [{ number: 1, head: { ref: 'config-change' }, base: { ref: 'main' } }]
+          }
+        }
+      }
+    }
+
+    it('ignores check runs created by another app', async () => {
+      await receive('check_run.created', run({ id: 2, slug: 'safe-settings-nonprod' }))
+
+      expect(github.rest.checks.update).not.toHaveBeenCalled()
+    })
+
+    it('processes check runs created by this app', async () => {
+      await receive('check_run.created', run({ id: 1, slug: 'safe-settings' }))
+
+      expect(github.rest.checks.update).toHaveBeenCalledWith(expect.objectContaining({ check_run_id: 42, status: 'in_progress' }))
+      expect(github.rest.checks.update).toHaveBeenLastCalledWith(expect.objectContaining({ check_run_id: 42, status: 'completed' }))
+    })
+
+    it('syncs a settings file that is only on the second page of changed files', async () => {
+      const syncSelectedRepos = jest.spyOn(Settings, 'syncSelectedRepos').mockResolvedValue()
+      const page1 = Array.from({ length: 100 }, (_, i) => ({ filename: `docs/file-${i}.md` }))
+      const page2 = [{ filename: `${env.CONFIG_PATH}/repos/my-repo.yml` }]
+      github.rest.pulls.listFiles.mockResolvedValue({ data: page1.slice(0, 30) })
+      github.paginate.mockImplementation(async (method, params) => {
+        if (method === github.rest.pulls.listFiles) return [...page1, ...page2]
+        return []
+      })
+
+      await receive('check_run.created', run({ id: 1, slug: 'safe-settings' }))
+
+      expect(github.paginate).toHaveBeenCalledWith(github.rest.pulls.listFiles, expect.objectContaining({ pull_number: 1, per_page: 100 }))
+      expect(syncSelectedRepos).toHaveBeenCalledTimes(1)
+      expect(syncSelectedRepos.mock.calls[0][2]).toEqual([{ repo: 'my-repo', owner: 'test-org' }])
+    })
+  })
+})