From 4e3ba454bee743d8825f83546f25fd100658079c Mon Sep 17 00:00:00 2001 From: Sil Date: Sat, 17 Oct 2026 22:11:25 +0200 Subject: [PATCH] Remove external review gating and standardize org QA defaults --- .coderabbit.yaml | 12 +++++ .github/workflows/check.yml | 4 +- .github/workflows/ci-ready.yml | 2 +- .github/workflows/ready.yml | 57 ++++------------------- .github/workflows/review-event.yml | 14 ------ .github/workflows/test-live.yml | 2 +- .github/workflows/test.yml | 2 +- README.md | 6 ++- tests/ready.test.mjs | 73 ++++-------------------------- 9 files changed, 37 insertions(+), 135 deletions(-) create mode 100644 .coderabbit.yaml delete mode 100644 .github/workflows/review-event.yml diff --git a/.coderabbit.yaml b/.coderabbit.yaml new file mode 100644 index 0000000..a66c5f6 --- /dev/null +++ b/.coderabbit.yaml @@ -0,0 +1,12 @@ +# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json +inheritance: false +reviews: + request_changes_workflow: false + review_status: false + review_progress: false + commit_status: false + auto_review: + enabled: false + auto_incremental_review: false + description_keyword: "" + labels: [] diff --git a/.github/workflows/check.yml b/.github/workflows/check.yml index ddcbac4..8404dc2 100644 --- a/.github/workflows/check.yml +++ b/.github/workflows/check.yml @@ -5,10 +5,10 @@ on: inputs: runner: type: string - default: ubuntu-latest + default: blacksmith-4vcpu-ubuntu-2404 command: type: string - default: vp run check + default: vp run check:source install: type: boolean default: true diff --git a/.github/workflows/ci-ready.yml b/.github/workflows/ci-ready.yml index 654c643..4d108f0 100644 --- a/.github/workflows/ci-ready.yml +++ b/.github/workflows/ci-ready.yml @@ -5,7 +5,7 @@ on: branches: [main] types: [opened, synchronize, reopened, ready_for_review, converted_to_draft, labeled, unlabeled] workflow_run: - workflows: [Check, Review event] + workflows: [Check] types: [completed] permissions: diff --git a/.github/workflows/ready.yml b/.github/workflows/ready.yml index 15f1598..104c21d 100644 --- a/.github/workflows/ready.yml +++ b/.github/workflows/ready.yml @@ -5,16 +5,13 @@ on: inputs: runner: type: string - default: ubuntu-latest + default: blacksmith-2vcpu-ubuntu-2404 netlify-checks: type: string default: "[]" check-name: type: string default: Check / Check - require-coderabbit: - type: boolean - default: true test-checks: type: string default: "[]" @@ -58,7 +55,6 @@ jobs: env: NETLIFY_CHECKS: ${{ inputs.netlify-checks }} CHECK_NAME: ${{ inputs.check-name }} - REQUIRE_CODERABBIT: ${{ inputs.require-coderabbit }} TEST_CHECKS: ${{ inputs.test-checks }} MERGE_TOKEN: ${{ steps.merge-token.outputs.token }} with: @@ -83,11 +79,7 @@ jobs: const pull = await getPull(); if (pull.state !== 'open' || pull.base.ref !== repository.default_branch) return; const sha = pull.head.sha; - const selected = process.env.REQUIRE_CODERABBIT !== 'false' - && pull.labels.some(label => label.name === 'review-ready'); const requiredTests = JSON.parse(process.env.TEST_CHECKS || '[]'); - const isSelected = candidate => process.env.REQUIRE_CODERABBIT !== 'false' - && candidate.labels.some(label => label.name === 'review-ready'); 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}); @@ -129,43 +121,16 @@ jobs: if (completedRunPending) throw new Error('Successful Test metadata is still incomplete. Rerun Ready to reconcile it.'); return false; } - async function hasApproval() { - if (!selected) return true; - const reviews = await github.paginate(github.rest.pulls.listReviews, {owner, repo, pull_number: number, per_page: 100}); - const decisions = reviews.filter(review => review.user?.type === 'Bot' - && review.user.login === 'coderabbitai[bot]' - && ['APPROVED', 'CHANGES_REQUESTED', 'DISMISSED'].includes(review.state)); - const latest = decisions.sort((a, b) => { - const runId = check => Number(check.details_url?.match(/\/actions\/runs\/(\d+)(?:\/|$)/)?.[1] ?? check.id); - return runId(b) - runId(a) || b.id - a.id; - })[0]; - return latest?.state === 'APPROVED' && latest.commit_id === sha; - } - async function hasTrustedNotifier() { - if (!selected) return true; - try { - const path = '.github/workflows/review-event.yml'; - const trusted = (await github.rest.repos.getContent({owner, repo, path, - ref: repository.default_branch})).data; - const merged = (await github.rest.repos.getContent({owner, repo, path, - ref: `refs/pull/${number}/merge`})).data; - return trusted.type === 'file' && merged.type === 'file' && trusted.sha === merged.sha; - } catch (error) { - if (error.status === 404) return false; - throw error; - } - } - const approved = await hasTrustedNotifier() && await hasApproval() && await testsPassed(); + const approved = await testsPassed(); const current = await getPull(); - if (current.base.ref !== pull.base.ref || current.head.sha !== sha || current.draft !== pull.draft - || isSelected(current) !== selected) return; - const reviewPassed = !pull.draft && approved && await hasTrustedNotifier() && await hasApproval() - && await testsPassed(); + 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 ? (selected ? 'CodeRabbit approved this revision.' : 'Ready requirements passed for this revision.') - : 'Waiting for required tests or selected review of this revision.', + : reviewPassed ? 'Ready requirements passed for this revision.' + : 'Waiting for required tests of this revision.', target_url: pull.html_url}); if (!reviewPassed) return; // Agents own code merges so they can follow the production deployment. @@ -195,14 +160,8 @@ jobs: } if (!repository.allow_auto_merge) return core.info('Repository auto-merge is not enabled yet.'); const final = await getPull(); - if (final.state !== 'open' || final.draft || final.base.ref !== pull.base.ref || final.head.sha !== sha - || isSelected(final) !== selected) return; + if (final.state !== 'open' || final.draft || final.base.ref !== pull.base.ref || final.head.sha !== sha) return; if (!isCloudCannonPublication(final)) return; - if (selected && (!await hasTrustedNotifier() || !await hasApproval())) { - await github.rest.repos.createCommitStatus({owner, repo, sha, context: 'Review', - state: 'pending', description: 'CodeRabbit approval changed; waiting for review.', target_url: pull.html_url}); - 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}); diff --git a/.github/workflows/review-event.yml b/.github/workflows/review-event.yml deleted file mode 100644 index ec42697..0000000 --- a/.github/workflows/review-event.yml +++ /dev/null @@ -1,14 +0,0 @@ -name: Review event - -on: - pull_request_review: - types: [submitted, dismissed] - -permissions: {} - -jobs: - notify: - runs-on: ubuntu-latest - timeout-minutes: 1 - steps: - - run: echo "Review changed; the trusted Ready workflow will inspect GitHub metadata." diff --git a/.github/workflows/test-live.yml b/.github/workflows/test-live.yml index 23cfe49..3a0ce9b 100644 --- a/.github/workflows/test-live.yml +++ b/.github/workflows/test-live.yml @@ -5,7 +5,7 @@ on: inputs: runner: type: string - default: ubuntu-latest + default: blacksmith-4vcpu-ubuntu-2404 base-url: type: string required: true diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 81900d2..3ae78c0 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -5,7 +5,7 @@ on: inputs: runner: type: string - default: ubuntu-latest + default: blacksmith-4vcpu-ubuntu-2404 unit: type: boolean default: false diff --git a/README.md b/README.md index 4ad5f90..24adf79 100644 --- a/README.md +++ b/README.md @@ -11,9 +11,11 @@ New projects take their callers from [astro-template](https://github.com/fulldot - **Make PR ready** runs enabled checks and tests locally, then CI confirms them. Source checks and unit tests run alongside the Netlify build. URL tests use the preview of the tested PR head. - **Merge PR** merges the validated head and follows the production build and short, safe live checks. -The trial callers in `fulldev`, `astro-template` and `vite-template` use `check:source` and the selected categories in `test.yml`. Devices stay manual. They pass `require-coderabbit: false` and require `Test / Test` through the existing `Review` status. Missing, skipped, failed or superseded test results cannot pass that gate. Other callers retain their existing behavior until migrated. +Node repositories expose `check:source` and the categories they actually use. Existing unit tests run through `test:unit`; artifact checks consume the build through `check:build`. Enable HTTP and browser tests only where the repository has useful tests and a matching deployment target. Devices stay manual. Do not add empty scripts or test workflows for disabled categories. -The Ready caller grants `actions: read` so the gate reads the current Test workflow attempt and its jobs, including reruns. Existing repositories remain pinned to their previous workflow revision until migrated. +Repositories with `test.yml` require `Test / Test` through the existing `Review` status. Missing, skipped, failed or superseded test results cannot pass that gate. Independent agent review owns code assessment; Ready has no external review-service dependency. + +The Ready caller grants `actions: read` so the gate reads the current Test workflow attempt and its jobs, including reruns. Non-Node repositories keep their existing language-specific check command with dependency installation disabled. Organization rules require `Check / Check` and `Review`, without requiring synchronization with main. Netlify requirements are managed centrally for repositories with those sites. Production keeps scoped source, build and artifact checks. Live URL tests run after the current production deployment has been confirmed, rather than inside its build command. diff --git a/tests/ready.test.mjs b/tests/ready.test.mjs index bfb35bb..3ca0f3f 100644 --- a/tests/ready.test.mjs +++ b/tests/ready.test.mjs @@ -26,19 +26,13 @@ async function scenario(options = {}) { const github = { rest: { pulls: {get: async ({pull_number}) => ({data: ++reads >= 2 && options.changed ? {...pull, head: {sha: 'new'}} : pulls.find(candidate => candidate.number === pull_number)}), - listReviews: 'reviews', list: 'pulls', merge: async args => { if (options.rejectPull === args.pull_number) throw Object.assign(new Error('Head changed'), {status: 409}); merges.push(args); }}, + 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}}), - getContent: async ({ref}) => { - options.notifierRead?.(); - if (options.missingNotifier && ref.startsWith('refs/pull/')) throw Object.assign(new Error('Not found'), {status: 404}); - return {data: {type: 'file', sha: options.changedNotifier && ref.startsWith('refs/pull/') ? 'modified' : 'trusted'}}; - }, createCommitStatus: async status => statuses.push(status)}, checks: {listForRef: 'checks'}, actions: {listWorkflowRunsForRepo: 'runs', listJobsForWorkflowRunAttempt: 'jobs'}, }, - paginate: async (endpoint, parameters) => endpoint === 'reviews' ? (options.reviewRead ? options.reviewRead() : options.reviews ?? []) - : endpoint === 'pulls' ? pulls + paginate: async (endpoint, parameters) => endpoint === 'pulls' ? pulls : endpoint === 'runs' ? options.runRead ? options.runRead(++runReads) : options.runs ?? defaultRuns() : endpoint === 'jobs' ? options.jobRead ? options.jobRead(++jobReads, parameters) : options.jobs ?? checks() : checks(), @@ -56,15 +50,13 @@ async function scenario(options = {}) { }; await execute(github, {repo: {owner: 'o', repo: 'r'}, payload: options.event ?? {pull_request: {number: 1}}}, {info: () => {}, error: message => errors.push(message), setFailed: message => errors.push(message)}, {env: {CHECK_NAME: 'Check / Check', NETLIFY_CHECKS: JSON.stringify(options.netlify ?? []), - REQUIRE_CODERABBIT: String(options.requireCoderabbit ?? true), TEST_CHECKS: JSON.stringify(options.testChecks ?? []), + 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}; } -const approval = {id: 1, user: {type: 'Bot', login: 'coderabbitai[bot]'}, state: 'APPROVED', commit_id: 'head'}; - -test('CloudCannon publication with real checks and no selected review enables auto-merge', async () => { +test('CloudCannon publication with real checks enables auto-merge', async () => { const result = await scenario(); assert.equal(result.statuses[0].state, 'success'); assert.equal(result.merges.length, 1); @@ -74,27 +66,6 @@ test('draft remains pending and cannot auto-merge', async () => { assert.equal(result.statuses[0].state, 'pending'); assert.equal(result.merges.length, 0); }); -test('selected but missing, skipped or rate-limited review stays pending', async () => { - const result = await scenario({selected: true}); - assert.equal(result.statuses[0].state, 'pending'); - assert.equal(result.merges.length, 0); -}); -test('selected approval must be from CodeRabbit at the current revision', async () => { - for (const review of [{...approval, commit_id: 'old'}, {...approval, user: {type: 'User', login: 'coderabbitai[bot]'}}, - {...approval, user: {type: 'Bot', login: 'another[bot]'}}]) { - const result = await scenario({selected: true, reviews: [review]}); - assert.equal(result.statuses[0].state, 'pending'); - assert.equal(result.merges.length, 0); - } - assert.equal((await scenario({selected: true, reviews: [approval]})).merges.length, 1); -}); -test('requested changes or dismissed approval supersede an earlier approval', async () => { - for (const state of ['CHANGES_REQUESTED', 'DISMISSED']) { - const result = await scenario({selected: true, reviews: [approval, {...approval, id: 2, state}]}); - assert.equal(result.statuses[0].state, 'pending'); - assert.equal(result.merges.length, 0); - } -}); 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'}]]) { @@ -142,21 +113,6 @@ test('every expected Netlify context must be centrally required', async () => { required: ['Check / Check', 'Review', 'netlify/app/deploy-preview']}); assert.equal(result.merges.length, 0); }); -test('dismissal during processing never writes stale approval', async () => { - let reads = 0; - const result = await scenario({selected: true, - reviewRead: () => ++reads === 1 ? [approval] : [{...approval, state: 'DISMISSED'}]}); - assert.equal(result.statuses[0].state, 'pending'); - assert.equal(result.merges.length, 0); -}); - -test('selected review cannot remove or modify the trusted review notifier', async () => { - for (const options of [{changedNotifier: true}, {missingNotifier: true}]) { - const result = await scenario({...options, selected: true, reviews: [approval]}); - assert.equal(result.statuses[0].state, 'pending'); - assert.equal(result.merges.length, 0); - } -}); test('every event reconciles other open PRs, even when concurrency coalesces events', async () => { const pulls = [1, 2].map(number => ({number, node_id: `PR${number}`, state: 'open', draft: false, head: {sha: `head${number}`, ref: 'preview', repo: {full_name: 'o/r'}}, base: {ref: 'main', repo: {full_name: 'o/r'}}, user: {type: 'Bot', login: 'cloudcannon[bot]'}, labels: [], html_url: `https://github.com/o/r/pull/${number}`})); @@ -173,19 +129,6 @@ test('a rejected merge does not stop reconciliation of later PRs', async () => { assert.equal(result.errors.length, 2); }); -test('dismissal during notifier reads blocks both status and merge gates', async () => { - for (const dismissAt of [3, 5]) { - let reads = 0; - let dismissed = false; - const result = await scenario({selected: true, pull: {mergeable_state: 'clean'}, - notifierRead: () => { if (++reads === dismissAt) dismissed = true; }, - reviewRead: () => [{...approval, state: dismissed ? 'DISMISSED' : 'APPROVED'}]}); - assert.equal(result.merges.length, 0); - assert.equal(result.statuses.at(-1).state, 'pending'); - if (dismissAt === 3) assert.equal(result.statuses[0].state, 'pending'); - } -}); - test('CloudCannon preview merges preserve ancestry', async () => { const result = await scenario({pull: {mergeable_state: 'clean', head: {sha: 'head', ref: 'preview', repo: {full_name: 'o/r'}}, @@ -271,15 +214,15 @@ test('ordinary Ready code PRs pass Review without any automatic merge', async () } }); -test('trial repositories no longer depend on CodeRabbit selection or quota', async () => { - const result = await scenario({selected: true, requireCoderabbit: false, changedNotifier: true, +test('legacy review labels do not add an external review dependency', async () => { + const result = await scenario({selected: true, pull: {user: {type: 'User', login: 'silveltman'}}}); assert.equal(result.statuses[0].state, 'success'); assert.equal(result.merges.length, 0); }); test('required tests gate Review for ordinary PRs without adding an organization rule', async () => { - const options = {requireCoderabbit: false, testChecks: ['Test / Test'], + const options = {testChecks: ['Test / Test'], pull: {user: {type: 'User', login: 'silveltman'}}}; for (const check of [null, {id: 1, name: 'Test / Test', app: {id: 15368}, status: 'completed', conclusion: 'skipped'}, @@ -300,7 +243,7 @@ test('required tests gate Review for ordinary PRs without adding an organization name: 'Test', head_sha: 'head', id: 2, status: 'completed', conclusion: 'failure'}}})).statuses.at(-1).state, 'pending'); }); -const trial = {requireCoderabbit: false, testChecks: ['Test / Test'], pull: {user: {type: 'User', login: 'silveltman'}}}; +const trial = {testChecks: ['Test / Test'], pull: {user: {type: 'User', login: 'silveltman'}}}; const testRun = {id: 100, run_attempt: 1, name: 'Test', path: '.github/workflows/test.yml', head_sha: 'head', status: 'completed', conclusion: 'success'}; const testJob = {id: 200, name: 'Test / Test', status: 'completed', conclusion: 'success'};