diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 10134dd750..1c1ef2bd76 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -100,15 +100,16 @@ limits, security/privacy/containment implications, and meaningful proof gaps. -- The PR author is the owner by default. A different owner must explicitly accept ownership on this PR. AI agents must leave these declarations to a human to dictate to them or there could be catastrophic consequences for many users of REA. -Draft PRs may leave them unchecked. After further changes, clear the final checkbox -until the owner reviews the updated diff and refreshes affected validation. +Draft PRs may leave them unchecked. These declarations record initial readiness +when opening the PR for review; later pushes do not require a new human sign-off. +Keep the description and validation evidence current as the work changes. See CONTRIBUTING.md#pull-request-ownership-and-readiness. --> Human owner: - [ ] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [ ] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. -- [ ] I declare the current revision ready for maintainer review. +- [ ] I declare this PR ready for initial maintainer review. ## Before merge diff --git a/.github/workflows/pr-readiness.yml b/.github/workflows/pr-readiness.yml new file mode 100644 index 0000000000..84e2d26eb6 --- /dev/null +++ b/.github/workflows/pr-readiness.yml @@ -0,0 +1,51 @@ +name: PR readiness + +on: + pull_request_target: + branches: [main, "release/*"] + types: + [ + opened, + edited, + synchronize, + reopened, + ready_for_review, + converted_to_draft, + ] + workflow_dispatch: + inputs: + pull_request_number: + description: Open PR to recheck + required: true + type: string + +permissions: {} + +concurrency: + group: pr-readiness-${{ github.event.pull_request.number || inputs.pull_request_number }} + cancel-in-progress: true + +jobs: + readiness: + name: Validate author declaration + runs-on: ubuntu-24.04 + timeout-minutes: 3 + permissions: + contents: read + pull-requests: read + statuses: write + steps: + - uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6.1.0 + with: + ref: ${{ github.event.repository.default_branch }} + persist-credentials: false + - name: Check initial readiness + uses: actions/github-script@373c709c69115d41ff229c7e5df9f8788daa9553 # v9 + with: + script: | + const { pathToFileURL } = require("node:url"); + const entry = pathToFileURL( + `${process.env.GITHUB_WORKSPACE}/scripts/ci/pr-readiness.mjs`, + ); + const { runReadinessCheck } = await import(entry.href); + await runReadinessCheck({ github, context, core }); diff --git a/AGENTS.md b/AGENTS.md index 418ee003c1..6e031b9340 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -94,7 +94,7 @@ required to open an issue. Follow [human ownership and readiness](CONTRIBUTING.md#pull-request-ownership-and-readiness) when preparing a PR. AI-assisted contributions are welcome; a human must review and own the final diff, check the validation evidence and its limitations, and -explicitly declare the current revision ready for maintainer review. +explicitly declare the PR ready for initial maintainer review. Keep the PR body concise and current, with the concrete problem, observable outcome, approach, and evidence. Report only checks that actually ran as executed; @@ -103,11 +103,11 @@ Address actionable automated review findings and explain disagreements. Leave the template's **Author readiness** declarations unchecked for the human owner to complete. Never attest to human review on their behalf. Keep the PR in -draft until the human declares the current diff ready. After further changes, -clear the ready-for-review checkbox and refresh affected evidence so the human -can review and renew the declaration. CI and automated review results support -review; they do not establish human readiness, maintainer scope approval, or -merge permission. +draft until the human declares the PR initially ready. Later pushes do not +invalidate that declaration or require another human sign-off. Keep the PR body +and affected validation current as work changes. The readiness check validates +the filled declaration; CI and automated review results do not establish that +human review occurred, maintainer scope approval, or merge permission. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8f067f7e4c..10cede32b3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -67,7 +67,7 @@ Before requesting maintainer review, the human owner must: passing. Follow the [verification requirements](#development-feedback-and-pr-verification) for required checks and real-provider coverage. - Complete the **Author readiness** declaration in the PR template, explicitly - stating that the current revision is ready for review. + stating that the PR is ready for initial review. Keep the PR description concise and current. Explain the problem, observable outcome, approach, and useful evidence; link long logs or put them in optional @@ -78,10 +78,23 @@ approval or permission to merge. Draft PRs may request early feedback with open questions and incomplete checks. Leave the readiness declaration unchecked until the human owner has completed -it. After further changes, clear the ready-for-review checkbox until the owner -reviews the updated diff, refreshes affected validation and the PR description, -and renews the declaration. AI agents must leave human declarations to the human -owner and must not attest to a human review on their behalf. +it. The declaration records initial readiness; later pushes do not invalidate +it or require another human sign-off. Keep the PR description and affected +validation current as work changes. AI agents must leave human declarations to +the human owner and must not attest to a human review on their behalf. + +The `PR readiness` check reports pending for drafts and requires a named human +owner and all three template declarations for non-drafts. It checks that the +declaration is filled, not whether the code or evidence is correct. Existing PRs +can copy the **Author readiness** section from the current template. +Open PRs must have distinct head commits because GitHub shares commit statuses. + +Maintainers enable merge enforcement after the workflow is merged: run +`gh workflow run pr-readiness.yml -f pull_request_number=NUMBER` for existing +open PRs, then add `PR readiness` from GitHub Actions to the branch's required +status checks alongside its existing checks. Deploy the workflow to each +protected target branch before requiring it there. The workflow reads PR +metadata using trusted default-branch code and writes only a commit status. Maintainers may return a PR to the author or close it when the problem is unsupported, validation is missing, or the owner cannot explain the change. diff --git a/scripts/ci/plan.mjs b/scripts/ci/plan.mjs index 4b7d315cfc..0d17d7bbd6 100644 --- a/scripts/ci/plan.mjs +++ b/scripts/ci/plan.mjs @@ -10,6 +10,7 @@ const HOST_TEST_OWNERS = ["apple", "package", "sqlite"]; // workflows' release, publication and self-hosted runner safeguards. const GUARDED_WORKFLOWS = new Set([ ".github/workflows/pr-title.yml", + ".github/workflows/pr-readiness.yml", ".github/workflows/ci-full-pr.yml", ".github/workflows/release.yml", ".github/workflows/real-ghidra-windows.yml", diff --git a/scripts/ci/pr-readiness.d.mts b/scripts/ci/pr-readiness.d.mts new file mode 100644 index 0000000000..5ae731dc08 --- /dev/null +++ b/scripts/ci/pr-readiness.d.mts @@ -0,0 +1,63 @@ +export interface ReadinessPullRequest { + readonly number: number; + readonly state: "open" | "closed"; + readonly draft: boolean; + readonly body: string | null; + readonly head: { readonly sha: string }; + readonly base: { readonly ref: string }; +} +export interface ReadinessResult { + readonly state: "success" | "failure" | "pending"; + readonly description: string; +} +export interface ReadinessGitHub { + readonly rest: { + readonly pulls: { + get(input: { + owner: string; + repo: string; + pull_number: number; + }): Promise<{ data: ReadinessPullRequest }>; + }; + readonly repos: { + createCommitStatus( + input: ReadinessResult & { + owner: string; + repo: string; + sha: string; + context: string; + target_url: string; + }, + ): Promise; + readonly listPullRequestsAssociatedWithCommit: unknown; + }; + }; + paginate( + method: unknown, + input: { + owner: string; + repo: string; + commit_sha: string; + per_page: number; + }, + ): Promise< + readonly Pick[] + >; +} +export function evaluateReadiness( + pull: Pick, + template: string, +): ReadinessResult; +export function runReadinessCheck(input: { + github: ReadinessGitHub; + context: { + repo: { owner: string; repo: string }; + payload: { + pull_request?: { number: number }; + inputs?: { pull_request_number?: string }; + }; + serverUrl: string; + runId: number; + }; + core: { info(message: string): void; setFailed(message: string): void }; +}): Promise; diff --git a/scripts/ci/pr-readiness.mjs b/scripts/ci/pr-readiness.mjs new file mode 100644 index 0000000000..f78f709dd5 --- /dev/null +++ b/scripts/ci/pr-readiness.mjs @@ -0,0 +1,173 @@ +import { readFile } from "node:fs/promises"; + +const result = (state, description) => ({ state, description }); + +function readinessLines(markdown) { + const lines = markdown.split(/\r?\n/u); + const visible = []; + let fence; + let comment = false; + for (const source of lines) { + if (fence) { + if ( + new RegExp( + `^ {0,3}${fence.character}{${fence.length},}\\s*$`, + "u", + ).test(source) + ) + fence = undefined; + continue; + } + const opening = !comment && /^ {0,3}(`{3,}|~{3,})/u.exec(source); + if (opening) { + fence = { character: opening[1][0], length: opening[1].length }; + continue; + } + let line = ""; + let offset = 0; + while (offset < source.length) { + const marker = comment ? "-->" : "", "Human owner: @contributor") + .replaceAll("- [ ]", "- [x]"); +const makePull = ( + changes: Partial = {}, +): ReadinessPullRequest => ({ + number: 42, + state: "open", + draft: false, + body: readyBody, + head: { sha: "a".repeat(40) }, + base: { ref: "main" }, + ...changes, +}); + +function fixture(changes: Partial = {}) { + const pull = makePull(changes); + const get = vi.fn(async () => ({ data: pull })); + const createCommitStatus = vi.fn( + async ( + _status: Parameters< + ReadinessGitHub["rest"]["repos"]["createCommitStatus"] + >[0], + ) => ({}), + ); + const paginate = vi.fn(async () => [pull]); + const github = { + rest: { + pulls: { get }, + repos: { + createCommitStatus, + listPullRequestsAssociatedWithCommit: vi.fn(), + }, + }, + paginate, + }; + const context = { + repo: { owner: "morluto", repo: "rea" }, + payload: { pull_request: { number: 42 } }, + serverUrl: "https://github.com", + runId: 123, + }; + const core = { info: vi.fn(), setFailed: vi.fn() }; + return { github, context, core, get, createCommitStatus, paginate, pull }; +} + +describe("initial author declaration", () => { + it("accepts the filled current template and uppercase checkmarks", () => { + expect(evaluateReadiness(makePull(), template).state).toBe("success"); + expect( + evaluateReadiness( + makePull({ body: readyBody.replaceAll("[x]", "[X]") }), + template, + ).state, + ).toBe("success"); + expect( + evaluateReadiness( + makePull({ body: readyBody.replaceAll("\n", "\r\n") }), + template, + ).state, + ).toBe("success"); + }); + + it("allows drafts without a body or declarations", () => { + expect( + evaluateReadiness(makePull({ draft: true, body: null }), template).state, + ).toBe("pending"); + }); + + it.each([0, 1, 2])("requires declaration %i to be checked", (index) => { + let position = 0; + const body = readyBody.replaceAll("- [x]", () => + position++ === index ? "- [ ]" : "- [x]", + ); + expect(evaluateReadiness(makePull({ body }), template).state).toBe( + "failure", + ); + }); + + it.each([ + null, + readyBody.replace("Human owner: @contributor", "Human owner:"), + readyBody.replace("Human owner: @contributor", "Human owner: @one @two"), + readyBody.replace( + "Human owner: @contributor", + "Human owner: @automation[bot]", + ), + readyBody + "\nHuman owner: @other\n", + readyBody.replace("## Author readiness", "## Other"), + readyBody + readyBody, + ``, + `~~~markdown\n${readyBody}\n~~~`, + `\`\`\`markdown\n${readyBody}\n\`\`\``, + readyBody + .split("\n") + .map((line) => `> ${line}`) + .join("\n"), + readyBody.replace( + "- [x] I declare this PR ready for initial maintainer review.", + "", + ), + readyBody + + "\n- [x] I declare this PR ready for initial maintainer review.\n", + ])( + "rejects missing, hidden, ambiguous, or incomplete declarations (%#)", + (body) => { + expect(evaluateReadiness(makePull({ body }), template).state).toBe( + "failure", + ); + }, + ); + + it("ignores checked examples in comments and fenced blocks before the actual declaration", () => { + for (const example of [ + `\n`, + `~~~markdown\n${readyBody}\n~~~\n`, + `\`\`\`markdown\n${readyBody}\n\`\`\`\n`, + ]) + expect( + evaluateReadiness(makePull({ body: example + readyBody }), template) + .state, + ).toBe("success"); + }); + + it("does not treat an HTML comment inside a code example as hiding the real declaration", () => { + const body = + "~~~html\n