diff --git a/.github/workflows/ready.yml b/.github/workflows/ready.yml index 104c21d..c5ea5f4 100644 --- a/.github/workflows/ready.yml +++ b/.github/workflows/ready.yml @@ -80,9 +80,27 @@ jobs: if (pull.state !== 'open' || pull.base.ref !== repository.default_branch) return; const sha = pull.head.sha; const requiredTests = JSON.parse(process.env.TEST_CHECKS || '[]'); - if (requiredTests.length) await github.rest.repos.createCommitStatus({owner, repo, sha, - context: 'Review', state: 'pending', description: 'Checking the current Test run and attempt.', - target_url: pull.html_url}); + const reviewTarget = {owner, repo, sha, context: 'Review', target_url: pull.html_url}; + let review; + async function setReview(state, description) { + // Pending already blocks publication, regardless of its wording. + if (review?.state === state && (state === 'pending' || review.description === description)) return; + await github.rest.repos.createCommitStatus({...reviewTarget, state, description}); + review = {state, description}; + } + try { + const result = await github.rest.repos.getCombinedStatusForRef({owner, repo, ref: sha, per_page: 100}); + review = result.data.statuses.find(status => status.context === 'Review'); + } catch (error) { + // A failed status read must not leave a previous success in force. + await setReview('pending', 'Unable to verify the current Review status.'); + throw error; + } + if (pull.draft) { + await setReview('pending', 'Draft: publication is not authorized.'); + return; + } + if (requiredTests.length) await setReview('pending', 'Checking the current Test run and attempt.'); async function testsPassed() { if (!requiredTests.length) return true; // Read the current run and attempt on every event. Check runs can @@ -126,12 +144,9 @@ jobs: if (current.state !== 'open' || current.base.ref !== pull.base.ref || current.head.sha !== sha || current.draft !== pull.draft) return; const reviewPassed = !pull.draft && approved && await testsPassed(); - await github.rest.repos.createCommitStatus({owner, repo, sha, context: 'Review', - state: reviewPassed ? 'success' : 'pending', - description: pull.draft ? 'Draft: publication is not authorized.' - : reviewPassed ? 'Ready requirements passed for this revision.' - : 'Waiting for required tests of this revision.', - target_url: pull.html_url}); + await setReview(reviewPassed ? 'success' : 'pending', reviewPassed + ? 'Ready requirements passed for this revision.' + : 'Waiting for required tests of this revision.'); if (!reviewPassed) return; // Agents own code merges so they can follow the production deployment. // Keep the customer's CloudCannon Publish button automatic. @@ -163,8 +178,7 @@ jobs: if (final.state !== 'open' || final.draft || final.base.ref !== pull.base.ref || final.head.sha !== sha) return; if (!isCloudCannonPublication(final)) return; if (!await testsPassed()) { - await github.rest.repos.createCommitStatus({owner, repo, sha, context: 'Review', - state: 'pending', description: 'Waiting for required tests of this revision.', target_url: pull.html_url}); + await setReview('pending', 'Waiting for required tests of this revision.'); return; } const method = final.head.ref === 'preview' && final.head.repo?.full_name === final.base.repo?.full_name diff --git a/tests/ready.test.mjs b/tests/ready.test.mjs index 3ca0f3f..1e9a7d0 100644 --- a/tests/ready.test.mjs +++ b/tests/ready.test.mjs @@ -28,6 +28,10 @@ async function scenario(options = {}) { pulls: {get: async ({pull_number}) => ({data: ++reads >= 2 && options.changed ? {...pull, head: {sha: 'new'}} : pulls.find(candidate => candidate.number === pull_number)}), list: 'pulls', merge: async args => { if (options.rejectPull === args.pull_number) throw Object.assign(new Error('Head changed'), {status: 409}); merges.push(args); }}, repos: {get: async () => ({data: {default_branch: 'main', full_name: 'o/r', allow_auto_merge: options.autoMerge !== false}}), + getCombinedStatusForRef: async () => { + if (options.statusError) throw new Error('Status unavailable'); + return {data: {statuses: options.review ? [{context: 'Review', ...options.review}] : []}}; + }, createCommitStatus: async status => statuses.push(status)}, checks: {listForRef: 'checks'}, actions: {listWorkflowRunsForRepo: 'runs', listJobsForWorkflowRunAttempt: 'jobs'}, @@ -53,7 +57,7 @@ async function scenario(options = {}) { TEST_CHECKS: JSON.stringify(options.testChecks ?? []), GITHUB_API_URL: 'https://api.github.com', MERGE_TOKEN: options.mergeToken}}, options.fetch ?? (() => { throw new Error('Unexpected network request'); }), resolve => resolve()); - return {statuses, merges, errors}; + return {statuses, merges, errors, runReads}; } test('CloudCannon publication with real checks enables auto-merge', async () => { @@ -66,6 +70,37 @@ test('draft remains pending and cannot auto-merge', async () => { assert.equal(result.statuses[0].state, 'pending'); assert.equal(result.merges.length, 0); }); +test('pending drafts do not append statuses or query Test metadata on repeated events', async () => { + for (const description of ['Draft: publication is not authorized.', 'Checking the current Test run and attempt.']) { + const result = await scenario({pull: {draft: true}, testChecks: ['Test / Test'], + review: {state: 'pending', description}, runRead: () => {throw new Error('Draft must not query Test');}}); + assert.deepEqual(result.statuses, []); + assert.equal(result.runReads, 0); + assert.deepEqual(result.errors, []); + assert.deepEqual(result.merges, []); + } +}); +test('converting a successful PR to draft revokes its old Review', async () => { + const result = await scenario({pull: {draft: true}, testChecks: ['Test / Test'], + review: {state: 'success'}, runRead: () => {throw new Error('Draft must not query Test');}}); + assert.equal(result.statuses.length, 1); + assert.equal(result.statuses[0].state, 'pending'); + assert.deepEqual(result.errors, []); + assert.deepEqual(result.merges, []); +}); +test('an already-pending ready PR waits without another status write', async () => { + const result = await scenario({testChecks: ['Test / Test'], review: {state: 'pending'}, + runs: [{id: 100, name: 'Test', path: '.github/workflows/test.yml', head_sha: 'head', status: 'in_progress'}]}); + assert.deepEqual(result.statuses, []); + assert.deepEqual(result.errors, []); + assert.deepEqual(result.merges, []); +}); +test('a status read failure revokes any previous success before failing Ready', async () => { + const result = await scenario({statusError: true, review: {state: 'success'}}); + assert.equal(result.statuses.at(-1).state, 'pending'); + assert.match(result.errors[0], /Status unavailable/); + assert.deepEqual(result.merges, []); +}); test('skipped, failed or absent static checks never authorize publication', async () => { for (const checks of [[], [{id: 1, name: 'Check / Check', app: {id: 15368}, conclusion: 'skipped'}], [{id: 1, name: 'Check / Check', app: {id: 15368}, conclusion: 'failure'}]]) { @@ -282,7 +317,8 @@ test('persistent missing metadata is bounded and API errors revoke old success', const missing = await scenario({...trial, runRead: () => {reads++; return [];}}); assert.equal(reads, 4); assert.equal(missing.statuses.at(-1).state, 'pending'); - const failed = await scenario({...trial, runRead: () => {throw new Error('Metadata unavailable');}}); + const failed = await scenario({...trial, review: {state: 'success'}, + runRead: () => {throw new Error('Metadata unavailable');}}); assert.equal(failed.statuses.at(-1).state, 'pending'); assert.match(failed.errors[0], /Metadata unavailable/); });