diff --git a/.github/workflows/pr-approved-issue.yml b/.github/workflows/pr-approved-issue.yml new file mode 100644 index 0000000..d06a7f2 --- /dev/null +++ b/.github/workflows/pr-approved-issue.yml @@ -0,0 +1,42 @@ +name: 'PR Approved Issue Check (Reusable)' +# Every PR from an external contributor is converted to a draft if its +# description does not link an approved issue assigned to the PR author. +# Repository owners, org members, collaborators and bots bypass this check. + +on: + workflow_call: + inputs: + ref: + description: 'Git ref (branch, tag, or commit SHA) of shared-workflows to check out' + required: false + type: string + default: 'main' + +permissions: + contents: write # Needed, with pull-requests: write, to toggle draft state. + issues: read + pull-requests: write + +jobs: + approved-issue: + # Bypass maintainers, collaborators and bots (e.g. dependabot). + if: >- + github.event.pull_request.user.type != 'Bot' && + !contains(fromJSON('["OWNER", "MEMBER", "COLLABORATOR"]'), github.event.pull_request.author_association) + runs-on: ubuntu-latest + steps: + - name: 'Check out shared workflows repo' + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + repository: 'keras-team/shared-workflows' + ref: '${{ inputs.ref }}' + persist-credentials: false + path: '.shared-workflows' + + - name: 'Check PR approved issue' + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + with: + github-token: '${{ secrets.GITHUB_TOKEN }}' + script: | + const prApprovedIssue = require('./.shared-workflows/scripts/pr-approved-issue.js'); + await prApprovedIssue({ github, context, core }); diff --git a/docs/pr_policy.md b/docs/pr_policy.md new file mode 100644 index 0000000..aaafff3 --- /dev/null +++ b/docs/pr_policy.md @@ -0,0 +1,51 @@ +# Keras Pull Request Policy + +To keep review bandwidth focused on high-leverage improvements and prevent duplicate work, external pull requests across `keras-team` repositories require a **linked, approved issue assigned to the PR author**. + +> **NOTE:** We're constantly evolving this policy for optimal project velocity. If you have any feedback, please chime in on [keras#23601](https://github.com/keras-team/keras/issues/23601). + +## Overview of Requirements + +1. **Open or find an issue first**: + - Before opening a pull request, find an existing issue or open a new issue describing the bug or feature. + - If an issue is open for contributions, comment on the issue to request assignment. +2. **Wait for issue assignment**: + - Once a maintainer approves the direction and **assigns the issue to you**, you can proceed with opening a PR. +3. **Link the assigned issue in your PR description**: + - Include a reference to the issue in your PR description (for example, `Fixes #xxx` under the `## Approved issue link` section). +4. **Complete the Contributor Agreement checklist**: + - Check all required boxes in the PR description's Contributor Agreement section. +5. **Stay responsive during review**: + - You may have up to **3 PRs** ready for review at a time (additional PRs should remain in draft). + - Feel free to run `/gemini review` on your PR to catch minor issues while waiting for a maintainer review. + - PRs with no activity for 7 days will be marked as stale and closed after 7 additional days. + +--- + +## Automated Enforcement (`pr-approved-issue`) + +Our automated workflow checks every pull request from external contributors when a PR is `opened`, `edited`, `reopened`, or marked `ready_for_review`: + +- **Who is checked**: All external contributors (`FIRST_TIME_CONTRIBUTOR`, `CONTRIBUTOR`, `NONE`). Repository owners (`OWNER`), organization members (`MEMBER`), collaborators (`COLLABORATOR`), and bots (`Bot` / `[bot]`) are exempt. +- **How issues are matched**: + - The workflow strips HTML comments (``) from the PR description and scans for issue references belonging to the same repository (`#xxx`, `owner/repo#xxx`, or `https://github.com/owner/repo/issues/xxx`). + - Pull request numbers are ignored — the linked number must be an **issue** in the repository, and your GitHub username must be in the issue's **Assignees** list. +- **Draft state behavior**: + - **When the check fails**: If your PR does not link an issue assigned to you and is not currently a draft, the workflow automatically converts the PR to **Draft**, posts a comment explaining how to resolve it, and fails the check. + - **When the check passes**: Once you link an issue assigned to you in the PR description, the status check passes (`✅`). If your PR was in **Draft**, it stays in draft until you click **Ready for review** yourself. + +--- + +## FAQ + +### My PR was converted to a draft — how do I get it reviewed? + +1. Ensure an issue for your change exists in the repository and has been **assigned to you** by a maintainer. +2. Edit your PR description to link that issue (e.g., `Fixes #xxx` under `## Approved issue link`). +3. Wait for the `PR approved issue check` status to pass (`✅`), then click **Ready for review** on your PR. + +### Multiple people offered to send a PR for an issue — how is assignment decided? + +1. In general, assignment is first-come, first-served, with the reporter of the issue given the first opportunity to submit a PR. +2. Maintainers may author PRs directly based on urgency and priority. +3. If an issue is assigned to a contributor and there is no PR or activity within 7 days, it may be reassigned to the next volunteer. diff --git a/scripts/pr-approved-issue.js b/scripts/pr-approved-issue.js new file mode 100644 index 0000000..92cd625 --- /dev/null +++ b/scripts/pr-approved-issue.js @@ -0,0 +1,171 @@ +/** + * @license + * Copyright 2026 The Keras Authors. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * ============================================================================= + */ + +/** + * Enforce that external PRs link an approved issue assigned to the author. + * + * - Strips HTML comments from the PR description and scans for issue references + * (`#xxx`, `owner/repo#xxx` or a full issue URL). + * - Passes when at least one referenced issue exists in this repo and has the + * PR author as an assignee. + * - When the check fails: converts non-draft PRs to draft, posts a comment (on + * `opened` or when converting to draft), and marks the check as failed. + * + * Maintainers, collaborators and bots are skipped by the workflow `if:`. + */ + +const POLICY_DOC_URL = + "https://github.com/keras-team/shared-workflows/blob/main/docs/pr_policy.md"; +const BYPASS_ASSOCIATIONS = ["OWNER", "MEMBER", "COLLABORATOR"]; +// Historical RFC announcement issue in keras-team/keras that was linked in PR templates. +const IGNORED_REPO_ISSUES = { + "keras-team/keras": new Set([23601]), +}; + +module.exports = async function prApprovedIssue({ github, context, core }) { + const payloadPr = context.payload.pull_request; + if (!payloadPr) { + core.info("Not a pull request payload."); + return; + } + + const { owner, repo } = context.repo; + const action = context.payload.action; + + // Fetch live PR state so re-running a failed workflow job reads the latest + // PR description and draft status rather than the frozen event payload. + const { data: pr } = await github.rest.pulls.get({ + owner, + repo, + pull_number: payloadPr.number, + }); + const author = pr.user.login; + + // Defence in depth: the workflow `if:` already filters these out. + if ( + pr.user.type === "Bot" || + author.toLowerCase().endsWith("[bot]") || + BYPASS_ASSOCIATIONS.includes(pr.author_association) + ) { + core.info(`Skipping #${pr.number}: ${author} is ${pr.author_association}.`); + return; + } + + const body = pr.body || ""; + + // 1. Find issues referenced in the description that are assigned to the author. + const referenced = findIssueNumbers(body, owner, repo); + const assigned = []; + const notAssigned = []; + for (const number of referenced) { + try { + const { data: issue } = await github.rest.issues.get({ owner, repo, issue_number: number }); + if (issue.pull_request) continue; // A PR reference, not an issue. + const assignees = (issue.assignees || []).map((a) => a.login.toLowerCase()); + (assignees.includes(author.toLowerCase()) ? assigned : notAssigned).push(number); + } catch (err) { + core.info(`Could not fetch issue #${number}: ${err.message}`); + } + } + + if (assigned.length > 0) { + core.info( + `Approved issue check passed for #${pr.number}: ${assigned.map((n) => `#${n}`).join(", ")} assigned to @${author}.` + ); + return; + } + + // 2. Check failed: convert non-draft PRs to draft. + const wasNotDraft = !pr.draft; + if (wasNotDraft) { + await convertToDraft(github, core, pr); + } + + // 3. Post a comment when the PR is opened or when converting a non-draft PR to draft. + if (action === "opened" || wasNotDraft) { + const message = [ + `❌ **Approved issue check failed.** This PR has been placed in **draft** until it links an approved issue assigned to @${author}.`, + "", + `Please review the [Keras Pull Request Policy](${POLICY_DOC_URL}). To resolve this:`, + "1. Find or open an issue for this change and ask a maintainer to approve it and assign it to you.", + "2. Link the issue in this PR's description (e.g. `Fixes #xxx`).", + "3. Once this check passes, mark the PR **Ready for review** when it is ready.", + "", + notAssigned.length + ? `Referenced issue(s) not assigned to @${author}: ${notAssigned.map((n) => `#${n}`).join(", ")}.` + : "No issue reference was found in the description.", + ].join("\n"); + + await github.rest.issues.createComment({ + owner, + repo, + issue_number: pr.number, + body: message, + }); + } + + core.setFailed(`No approved issue assigned to ${author} is linked in #${pr.number}.`); +}; + +/** Collect issue numbers referenced in `body` (excluding HTML comments) that belong to this repo. */ +function findIssueNumbers(body, owner, repo) { + // Strip HTML comments so placeholders like are never matched. + const uncommented = body.replace(//g, ""); + const repoKey = `${owner}/${repo}`.toLowerCase(); + const ignored = IGNORED_REPO_ISSUES[repoKey] || new Set(); + + const escaped = `${owner}/${repo}`.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + const patterns = [ + new RegExp(`https?://github\\.com/${escaped}/issues/(\\d+)`, "gi"), + new RegExp(`(?:^|[^\\w/])${escaped}#(\\d+)`, "gi"), + // Bare `#123`, but not `owner/repo#123` of some other repo. + /(?:^|[^\w/])#(\d+)\b/g, + ]; + const numbers = new Set(); + for (const pattern of patterns) { + for (const match of uncommented.matchAll(pattern)) { + const num = Number(match[1]); + if (!ignored.has(num)) { + numbers.add(num); + } + } + } + return [...numbers]; +} + +async function convertToDraft(github, core, pr) { + try { + await github.graphql( + `mutation($id: ID!) { + convertPullRequestToDraft(input: {pullRequestId: $id}) { + pullRequest { isDraft } + } + }`, + { id: pr.node_id } + ); + core.info(`Converted #${pr.number} to draft.`); + return true; + } catch (err) { + const lines = [`Could not convert #${pr.number} to draft: ${err.message}`]; + if (/not accessible by integration/i.test(err.message)) { + lines.push("Toggling draft state needs `contents: write` and `pull-requests: write`."); + } + core.warning(lines.join("\n")); + return pr.draft; + } +} diff --git a/scripts/pr-approved-issue.test.js b/scripts/pr-approved-issue.test.js new file mode 100644 index 0000000..7a5c7f2 --- /dev/null +++ b/scripts/pr-approved-issue.test.js @@ -0,0 +1,333 @@ +/** + * @license + * Copyright 2026 The Keras Authors. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * ============================================================================= + */ + +const { describe, it } = require('node:test'); +const assert = require('node:assert'); +const prApprovedIssue = require('./pr-approved-issue.js'); + +describe('prApprovedIssue', () => { + function createHarness({ + owner = 'keras-team', + repo = 'keras-hub', + action = 'opened', + pr = null, + payloadPr = undefined, + issuesByNumber = {}, + graphqlError = null, + }) { + const infoMessages = []; + const warningMessages = []; + let failedMessage = null; + const fetchedIssues = []; + const graphqlCalls = []; + const createdComments = []; + + const core = { + info: (msg) => infoMessages.push(msg), + warning: (msg) => warningMessages.push(msg), + setFailed: (msg) => { + failedMessage = msg; + }, + }; + + const github = { + rest: { + pulls: { + get: async () => ({ data: pr }), + }, + issues: { + get: async ({ issue_number }) => { + fetchedIssues.push(issue_number); + if (!(issue_number in issuesByNumber)) { + throw new Error('Not Found'); + } + return { data: issuesByNumber[issue_number] }; + }, + createComment: async (params) => { + createdComments.push(params); + }, + }, + }, + graphql: async (query, variables) => { + graphqlCalls.push({ query, variables }); + if (graphqlError) { + throw graphqlError; + } + return {}; + }, + }; + + const context = { + repo: { owner, repo }, + payload: { + action, + pull_request: payloadPr !== undefined ? payloadPr : (pr ? { number: pr.number } : null), + }, + }; + + return { + run: () => prApprovedIssue({ github, context, core }), + getInfoMessages: () => infoMessages, + getWarningMessages: () => warningMessages, + getFailedMessage: () => failedMessage, + getFetchedIssues: () => fetchedIssues, + getGraphqlCalls: () => graphqlCalls, + getCreatedComments: () => createdComments, + }; + } + + it('ignores non-pull-request payloads safely', async () => { + const harness = createHarness({ pr: null }); + await harness.run(); + assert.strictEqual(harness.getFailedMessage(), null); + assert.ok(harness.getInfoMessages().includes('Not a pull request payload.')); + }); + + it('skips bots and bypassed author associations (OWNER, MEMBER, COLLABORATOR)', async () => { + for (const [login, type, association] of [ + ['bot-account', 'Bot', 'NONE'], + ['dependabot[bot]', 'User', 'NONE'], + ['owner-user', 'User', 'OWNER'], + ['member-user', 'User', 'MEMBER'], + ['collab-user', 'User', 'COLLABORATOR'], + ]) { + const harness = createHarness({ + pr: { + number: 10, + node_id: 'PR_10', + draft: false, + author_association: association, + user: { login, type }, + body: 'No issue link needed', + }, + }); + await harness.run(); + assert.strictEqual(harness.getFailedMessage(), null); + assert.strictEqual(harness.getGraphqlCalls().length, 0); + assert.strictEqual(harness.getCreatedComments().length, 0); + } + }); + + it('fetches live PR state via pulls.get instead of relying on frozen event payload when re-run', async () => { + const harness = createHarness({ + action: 'opened', + // Stale frozen webhook payload from when the PR was first opened without an issue link: + payloadPr: { + number: 42, + node_id: 'PR_42', + draft: false, + author_association: 'NONE', + user: { login: 'external-dev', type: 'User' }, + body: 'Initial body without issue link', + }, + // Live PR state on GitHub after the author updated the description: + pr: { + number: 42, + node_id: 'PR_42', + draft: true, + author_association: 'NONE', + user: { login: 'external-dev', type: 'User' }, + body: 'Fixes #123', + }, + issuesByNumber: { + 123: { number: 123, assignees: [{ login: 'external-dev' }] }, + }, + }); + + await harness.run(); + + assert.strictEqual(harness.getFailedMessage(), null); + assert.strictEqual(harness.getGraphqlCalls().length, 0); + assert.strictEqual(harness.getCreatedComments().length, 0); + }); + + it('on opened without linked issue: converts non-draft PR to draft, posts comment, and fails', async () => { + const harness = createHarness({ + action: 'opened', + pr: { + number: 42, + node_id: 'PR_42', + draft: false, + author_association: 'NONE', + user: { login: 'external-dev', type: 'User' }, + body: 'Here is my new feature.', + }, + }); + + await harness.run(); + + // 1. Converted to draft via GraphQL. + assert.strictEqual(harness.getGraphqlCalls().length, 1); + assert.ok(harness.getGraphqlCalls()[0].query.includes('convertPullRequestToDraft')); + assert.deepStrictEqual(harness.getGraphqlCalls()[0].variables, { id: 'PR_42' }); + + // 2. Comment posted. + assert.strictEqual(harness.getCreatedComments().length, 1); + assert.ok(harness.getCreatedComments()[0].body.includes('No issue reference was found in the description.')); + + // 3. Workflow marked failed. + assert.strictEqual( + harness.getFailedMessage(), + 'No approved issue assigned to external-dev is linked in #42.' + ); + }); + + it('passes check without posting a comment or mutating draft state when an assigned issue is linked', async () => { + const harness = createHarness({ + action: 'edited', + pr: { + number: 42, + node_id: 'PR_42', + draft: true, + author_association: 'FIRST_TIME_CONTRIBUTOR', + user: { login: 'External-Dev', type: 'User' }, + body: '## Approved issue link\nFixes #123\nAlso see keras-team/keras-hub#124 and https://github.com/keras-team/keras-hub/issues/125', + }, + issuesByNumber: { + 123: { number: 123, assignees: [{ login: 'external-dev' }] }, + 124: { number: 124, assignees: [{ login: 'someone-else' }] }, + 125: { number: 125, assignees: [{ login: 'EXTERNAL-DEV' }] }, + }, + }); + + await harness.run(); + + // No GraphQL draft toggle and no comment when passing. + assert.strictEqual(harness.getGraphqlCalls().length, 0); + assert.strictEqual(harness.getCreatedComments().length, 0); + assert.strictEqual(harness.getFailedMessage(), null); + assert.ok( + harness + .getInfoMessages() + .includes('Approved issue check passed for #42: #125, #123 assigned to @External-Dev.') + ); + }); + + it('ignores issue references inside HTML comments and the RFC policy issue #23601', async () => { + const harness = createHarness({ + owner: 'keras-team', + repo: 'keras', + action: 'opened', + pr: { + number: 50, + node_id: 'PR_50', + draft: true, + author_association: 'NONE', + user: { login: 'contributor', type: 'User' }, + body: [ + '', + 'Please review our [PR Contribution Policy](https://github.com/keras-team/keras/issues/23601).', + 'Fixes other-org/other-repo#100 and https://github.com/other-org/other-repo/issues/101 and #200', + ].join('\n'), + }, + issuesByNumber: { + 123: { number: 123, assignees: [{ login: 'contributor' }] }, + 23601: { number: 23601, assignees: [{ login: 'contributor' }] }, + 100: { number: 100, assignees: [{ login: 'contributor' }] }, + 101: { number: 101, assignees: [{ login: 'contributor' }] }, + // #200 is a PR, not an issue. + 200: { number: 200, pull_request: {}, assignees: [{ login: 'contributor' }] }, + }, + }); + + await harness.run(); + + // Only #200 was fetched (neither #123 in HTML comment nor #23601 RFC policy issue was fetched). + assert.deepStrictEqual(harness.getFetchedIssues(), [200]); + assert.strictEqual(harness.getGraphqlCalls().length, 0); + assert.notStrictEqual(harness.getFailedMessage(), null); + assert.ok( + harness.getCreatedComments()[0].body.includes('No issue reference was found in the description.') + ); + }); + + it('converts non-draft PR to draft and posts comment when edited to reference only unassigned issues', async () => { + const harness = createHarness({ + action: 'edited', + pr: { + number: 55, + node_id: 'PR_55', + draft: false, + author_association: 'CONTRIBUTOR', + user: { login: 'contributor', type: 'User' }, + body: '## Approved issue link\nFixes #300 and #999', + }, + issuesByNumber: { + 300: { number: 300, assignees: [{ login: 'other-user' }] }, + // #999 does not exist (throws Not Found). + }, + }); + + await harness.run(); + + assert.strictEqual(harness.getGraphqlCalls().length, 1); + assert.ok(harness.getGraphqlCalls()[0].query.includes('convertPullRequestToDraft')); + assert.strictEqual(harness.getCreatedComments().length, 1); + assert.ok( + harness + .getCreatedComments()[0] + .body.includes('Referenced issue(s) not assigned to @contributor: #300.') + ); + assert.notStrictEqual(harness.getFailedMessage(), null); + }); + + it('does not post duplicate comments when an already-draft PR is edited and still failing', async () => { + const harness = createHarness({ + action: 'edited', + pr: { + number: 60, + node_id: 'PR_60', + draft: true, + author_association: 'NONE', + user: { login: 'contributor', type: 'User' }, + body: 'Still working on description', + }, + }); + + await harness.run(); + + assert.strictEqual(harness.getGraphqlCalls().length, 0); + assert.strictEqual(harness.getCreatedComments().length, 0); + assert.notStrictEqual(harness.getFailedMessage(), null); + }); + + it('logs warning with permissions hint when GraphQL convertToDraft fails', async () => { + const harness = createHarness({ + action: 'ready_for_review', + pr: { + number: 70, + node_id: 'PR_70', + draft: false, + author_association: 'NONE', + user: { login: 'contributor', type: 'User' }, + body: 'No issue linked yet', + }, + graphqlError: new Error('Resource not accessible by integration'), + }); + + await harness.run(); + + assert.strictEqual(harness.getWarningMessages().length, 1); + assert.ok( + harness + .getWarningMessages()[0] + .includes('Toggling draft state needs `contents: write` and `pull-requests: write`.') + ); + assert.strictEqual(harness.getCreatedComments().length, 1); + }); +});