From c042e33063f0010c151361deaed26bd577c15ac3 Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Thu, 8 Oct 2026 16:57:02 +0200 Subject: [PATCH] ci: require accepted issues for community pull requests Evaluate issue acceptance using trusted default-branch code and the PR author's repository permissions. Report the native workflow job result, protect enforcement files, and align PR creation and review guidance. Closes #4339 Signed-off-by: Evan Lezar --- .agents/skills/build-from-issue/SKILL.md | 2 +- .agents/skills/create-github-pr/SKILL.md | 23 +++ .agents/skills/review-github-pr/SKILL.md | 19 ++- .github/CODEOWNERS | 6 +- .github/PULL_REQUEST_TEMPLATE.md | 2 +- .github/workflows/pr-issue-acceptance.yml | 40 ++++++ .github/zizmor.yml | 1 + AGENTS.md | 4 + CONTRIBUTING.md | 6 +- tasks/scripts/check_issue_acceptance.cjs | 77 ++++++++++ tasks/scripts/check_issue_acceptance.test.cjs | 131 ++++++++++++++++++ 11 files changed, 306 insertions(+), 5 deletions(-) create mode 100644 .github/workflows/pr-issue-acceptance.yml create mode 100644 tasks/scripts/check_issue_acceptance.cjs create mode 100644 tasks/scripts/check_issue_acceptance.test.cjs diff --git a/.agents/skills/build-from-issue/SKILL.md b/.agents/skills/build-from-issue/SKILL.md index d2c7ec07ea..16ef5bbd14 100644 --- a/.agents/skills/build-from-issue/SKILL.md +++ b/.agents/skills/build-from-issue/SKILL.md @@ -27,7 +27,7 @@ Use a single issue comment beginning with `> **🏗️ build-plan**` when a plan 1. Check the current branch and working tree. Preserve unrelated work. Create an issue-specific branch or worktree as needed, following Branch Names in `CONTRIBUTING.md`. 2. Implement the smallest coherent change that fulfills the acceptance criteria. Update relevant skills when behavior or commands change. Keep published documentation minimal: explain exactly what users need, avoid duplication across pages, and omit internal details with no user impact. 3. Add meaningful tests for changed behavior and follow the verification guidance in `CONTRIBUTING.md`. Select format, lint, compile or type checks, and tests for affected components and their dependencies. Run the relevant E2E lane for infrastructure, sandbox, or policy changes. Guidance and template edits need applicable Markdown, YAML, link, and consistency checks. Do not require full Rust, SDK, or repository CI solely because a commit or PR is being created; broaden checks only for a concrete remaining risk or failed check. -4. Review the diff, use a signed-off Conventional Commit, and prepare a PR following `create-github-pr`. +4. Before pushing, apply the contributor access and issue acceptance check in `create-github-pr`. If blocked, preserve local work and report the reason. Review the diff, use a signed-off Conventional Commit, and prepare the PR. Every PR from this issue-backed workflow must have its own existing issue and use `Closes #` in its Related Issue section. For work needing multiple PRs, split the scope into a closable issue per PR. A high-level issue may track those issues but should not be closed by an incomplete PR. Report the implementation, verification, and any remaining limitation in the PR description, rather than copying earlier issue diagnostics. diff --git a/.agents/skills/create-github-pr/SKILL.md b/.agents/skills/create-github-pr/SKILL.md index 4f06a0818c..919e139825 100644 --- a/.agents/skills/create-github-pr/SKILL.md +++ b/.agents/skills/create-github-pr/SKILL.md @@ -17,6 +17,26 @@ Create pull requests on GitHub using the `gh` CLI. ## Before Creating a PR +### Verify Contributor Access and Issue Acceptance + +Before pushing or creating a PR (including a draft), verify the author's access +to the target repository, not their fork. For the authenticated `gh` user: + +```bash +gh repo view OWNER/REPO --json nameWithOwner,viewerPermission +gh issue view ISSUE_NUMBER --repo OWNER/REPO --json number,url,labels +``` + +Authors with verified `WRITE`, `MAINTAIN`, or `ADMIN` access may proceed without +`state:accepted`; keep the issue link and note the missing label. Everyone else +must link at least one target-repository issue and have `state:accepted` on each. +Otherwise, preserve local work and report which issue needs acceptance. A direct +request, roadmap placement, other labels, or vouch status does not bypass this gate. + +When acting for another author, verify their permissions. Resolve failed lookups +before proceeding; retry sandbox network failures outside the sandbox before +reporting an authentication problem. Vouch requirements still apply separately. + ### Check Config Documentation If the branch changes gateway TOML parsing, `[openshell.gateway]` fields, @@ -220,6 +240,9 @@ EOF The command outputs the PR URL and number. +Rerun `PR Issue Acceptance` after linked issue labels change; it does not refresh +automatically on issue-label events. + **Display the URL using markdown link syntax** so it's easily clickable: ``` diff --git a/.agents/skills/review-github-pr/SKILL.md b/.agents/skills/review-github-pr/SKILL.md index 3903e03271..d69c891281 100644 --- a/.agents/skills/review-github-pr/SKILL.md +++ b/.agents/skills/review-github-pr/SKILL.md @@ -39,11 +39,28 @@ gh pr list --head "" --state open Retrieve the PR metadata: ```bash -gh pr view --json title,body,state,headRefName,baseRefName,labels,author +gh pr view --json title,body,state,headRefName,baseRefName,labels,author,closingIssuesReferences ``` Record the **title**, **body**, **headRefName**, and **baseRefName** for use in later steps. +### Check Issue Acceptance + +Inspect every closing issue reference (paginate as needed) and fetch its current +repository and labels. Verify the PR author's permission in the target repository: + +```bash +gh api repos/OWNER/REPO/collaborators/AUTHOR/permission --jq .permission +``` + +Call out each linked issue missing `state:accepted`, with an issue link. Verified +`write`, `maintain`, or `admin` access permits the exception; report it as context, +not a blocking concern. Other authors need at least one target-repository issue, +and every linked issue must be accepted; report violations under **Potential +Concerns**. PR body text such as `No issue required` does not bypass this gate. +Report failed lookups as unverified and continue the code review. For a branch +without a PR, note that issue acceptance was not verified. + ## Step 3: Generate the Diff ### Primary: gh pr diff diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 14f2e5ef61..07acb32b0a 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -6,7 +6,7 @@ ## Merge enforcement — these paths decide who can merge, so they notify the full # list of maintainers to raise scrutiny on changes here. -# +# # PR checks run local to the branch, so unless the check specifically avoids # using branch-local files, a PR could change its own gates and appear to pass # them. Covering the required checks here closes that gap. @@ -31,5 +31,9 @@ /tasks/scripts/check_maintainer_approval_test.py @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr /tasks/scripts/alert_maintainer_change_test.py @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr +# PR issue acceptance checks +/tasks/scripts/check_issue_acceptance.cjs @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr +/tasks/scripts/check_issue_acceptance.test.cjs @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr + # Auditable SBOM check /tasks/scripts/verify-image-sbom.sh @NVIDIA/openshell-codeowners @mrunalp @sjenning @derekwaynecarr diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index d479fd6aca..5d73a68819 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -2,7 +2,7 @@ ## Related Issue - + ## Changes diff --git a/.github/workflows/pr-issue-acceptance.yml b/.github/workflows/pr-issue-acceptance.yml new file mode 100644 index 0000000000..742647d763 --- /dev/null +++ b/.github/workflows/pr-issue-acceptance.yml @@ -0,0 +1,40 @@ +# SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +name: PR Issue Acceptance + +on: + pull_request_target: + types: [opened, edited, reopened, synchronize] + +permissions: + contents: read + issues: read + pull-requests: read + +concurrency: + group: pr-issue-acceptance-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + issue-acceptance: + name: OpenShell / PR Issue Acceptance + timeout-minutes: 5 + if: github.repository_owner == 'NVIDIA' + runs-on: ubuntu-latest + steps: + # pull_request_target supplies the trusted default-branch SHA. Load + # only that helper; PR metadata is data, never executable code. + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: ${{ github.sha }} + persist-credentials: false + sparse-checkout: tasks/scripts/check_issue_acceptance.cjs + sparse-checkout-cone-mode: false + + - name: Check author access and linked issue acceptance + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 + with: + script: | + const check = require('./tasks/scripts/check_issue_acceptance.cjs'); + await check({ github, context, core }); diff --git a/.github/zizmor.yml b/.github/zizmor.yml index 27a89f878e..5e19eaefa9 100644 --- a/.github/zizmor.yml +++ b/.github/zizmor.yml @@ -9,6 +9,7 @@ rules: - dco.yml:3 - e2e-label-help.yml:13 - maintainers-change-alert.yml:6 + - pr-issue-acceptance.yml:6 - release-canary.yml:3 - required-ci-gates.yml:3 - vouch-check.yml:3 diff --git a/AGENTS.md b/AGENTS.md index 68e2324e11..f703fd2424 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -174,6 +174,10 @@ ocsf_emit!(event); Follow the [branch naming convention in CONTRIBUTING.md](CONTRIBUTING.md#branch-names). +## Pull Requests + +Follow `CONTRIBUTING.md` and `create-github-pr`. Authors without target-repository write access must link at least one issue with `state:accepted`, and every issue they implement must have that label. Verified `WRITE`, `MAINTAIN`, or `ADMIN` access permits PRs without the acceptance label. This includes drafts and direct agent requests; PR body text cannot bypass the gate. + ## Commits - Always use [Conventional Commits](https://www.conventionalcommits.org/) format for commit messages diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4bc2fb5c6a..d18317a985 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -394,7 +394,11 @@ See [docs/CONTRIBUTING.mdx](docs/CONTRIBUTING.mdx) for the current docs authorin Every PR except an automated dependency update must close an existing issue. In the PR's **Related Issue** section, use `Closes #NNN` for the issue covering that PR's scope. Split multi-PR work into a closable issue for each PR; a tracking issue can link them. Security fixes follow the private disclosure process in [SECURITY.md](SECURITY.md). -Automated dependency update PRs, including Dependabot PRs, are exempt from the separate issue and closing-reference requirement. Human-authored dependency updates follow the normal issue requirement. The exception does not change applicable verification, review, or private vulnerability disclosure requirements. +Authors without write access to the target repository must link at least one issue with `state:accepted`, and every linked issue must be accepted. This applies to drafts and direct requests to agents. Authors with verified `WRITE`, `MAINTAIN`, or `ADMIN` access may create a PR without the acceptance label; they must still follow the issue-linking requirement above. PR body text, roadmap placement, other labels, and vouch status do not bypass acceptance. + +The `PR Issue Acceptance` workflow reports the native `OpenShell / PR Issue Acceptance` job result for the triggering PR when it is opened, edited, reopened, or updated with new commits. It does not publish a shared commit status; lookup and execution errors fail the job. Link accepted issues in this repository using `Fixes #NNN` or `Closes #NNN`; GitHub recognizes closing references when a PR targets the default branch. Rerun the workflow after issue labels change. Making the job required for merging and configuring any required Actions event policy are separate repository settings; merge-queue support is not included. + +Automated dependency update PRs, including Dependabot PRs, are exempt from the separate issue and closing-reference requirement. Human-authored dependency updates follow the normal issue requirement. The exception does not bypass `PR Issue Acceptance` for authors without repository write access or change applicable verification, review, or private vulnerability disclosure requirements. ### Branch Names diff --git a/tasks/scripts/check_issue_acceptance.cjs b/tasks/scripts/check_issue_acceptance.cjs new file mode 100644 index 0000000000..8b81ef82fc --- /dev/null +++ b/tasks/scripts/check_issue_acceptance.cjs @@ -0,0 +1,77 @@ +// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +module.exports = async function checkIssueAcceptance({ github, context, core }) { + const { owner, repo } = context.repo; + const { data: pr } = await github.rest.pulls.get({ + owner, repo, pull_number: context.payload.pull_request.number, + }); + const author = pr.user.login; + + async function evaluate() { + try { + const { data } = await github.rest.repos.getCollaboratorPermissionLevel({ + owner, repo, username: author, + }); + // The REST API maps maintain to write and triage to read. + if (['write', 'maintain', 'admin'].includes(data.permission)) { + core.info(`${author} has repository write access; acceptance gate passed.`); + return; + } + } catch (error) { + if (error.status !== 404) throw error; + } + + const issues = []; + let cursor = null; + do { + const result = await github.graphql(` + query($owner: String!, $repo: String!, $number: Int!, $cursor: String) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { + closingIssuesReferences(first: 100, after: $cursor) { + nodes { number url repository { name owner { login } } } + pageInfo { hasNextPage endCursor } + } + } + } + } + `, { owner, repo, number: pr.number, cursor }); + const linked = result.repository.pullRequest.closingIssuesReferences; + issues.push(...linked.nodes); + cursor = linked.pageInfo.hasNextPage ? linked.pageInfo.endCursor : null; + } while (cursor); + + if (issues.length === 0) { + return 'Community PRs must link an issue with state:accepted using ' + + 'Fixes #NNN or Closes #NNN.'; + } + + const unaccepted = []; + for (const issue of issues) { + if (`${issue.repository.owner.login}/${issue.repository.name}`.toLowerCase() + !== `${owner}/${repo}`.toLowerCase()) { + return `Acceptance must be recorded in ${owner}/${repo}, not ${issue.url}.`; + } + const labels = await github.paginate(github.rest.issues.listLabelsOnIssue, { + owner: issue.repository.owner.login, + repo: issue.repository.name, + issue_number: issue.number, + per_page: 100, + }); + if (!labels.some(label => label.name === 'state:accepted')) { + unaccepted.push(issue.url); + } + } + + if (unaccepted.length) { + return 'Community PRs require state:accepted on every linked issue. ' + + `Ask a maintainer to accept: ${unaccepted.join(', ')}. ` + + 'After acceptance, rerun this workflow.'; + } + core.info(`All ${issues.length} linked issue(s) have state:accepted.`); + } + + const failure = await evaluate(); + if (failure) core.setFailed(failure); +}; diff --git a/tasks/scripts/check_issue_acceptance.test.cjs b/tasks/scripts/check_issue_acceptance.test.cjs new file mode 100644 index 0000000000..443c8edaf9 --- /dev/null +++ b/tasks/scripts/check_issue_acceptance.test.cjs @@ -0,0 +1,131 @@ +// SPDX-FileCopyrightText: Copyright (c) 2025-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +const assert = require('node:assert/strict'); +const test = require('node:test'); +const checkIssueAcceptance = require('./check_issue_acceptance.cjs'); + +async function check({ + permission = 'read', issues = [], body = '', permissionError, + number = 7, headSha = 'pr-head', pullError, issueError, labelError, +} = {}) { + const failures = []; + const github = { + rest: { + pulls: { + get: async ({ pull_number }) => { + assert.equal(pull_number, number); + if (pullError) throw new Error('PR lookup failed'); + return { data: { + number, user: { login: 'contributor' }, head: { sha: headSha }, body, + } }; + }, + }, + repos: { + getCollaboratorPermissionLevel: async ({ username }) => { + assert.equal(username, 'contributor'); + if (permissionError) throw Object.assign(new Error('lookup failed'), { status: permissionError }); + return { data: { permission } }; + }, + createCommitStatus() { assert.fail('PR decisions must not publish commit statuses'); }, + }, + issues: { listLabelsOnIssue() {} }, + }, + graphql: async (_query, variables) => { + assert.equal(variables.number, number); + if (issueError) throw new Error('Issue lookup failed'); + return { repository: { pullRequest: { closingIssuesReferences: { + nodes: issues.map(issue => ({ + number: issue.number, + url: `https://github.com/${issue.owner || 'NVIDIA'}/OpenShell/issues/${issue.number}`, + repository: { name: 'OpenShell', owner: { login: issue.owner || 'NVIDIA' } }, + })), + pageInfo: { hasNextPage: false, endCursor: null }, + } } } }; + }, + paginate: async (_method, { issue_number }) => { + if (labelError) throw new Error('Label lookup failed'); + return issues.find(issue => issue.number === issue_number).labels || []; + }, + }; + await checkIssueAcceptance({ + github, + context: { + repo: { owner: 'NVIDIA', repo: 'OpenShell' }, + payload: { pull_request: { number } }, + actor: 'maintainer', sha: 'base-commit', serverUrl: 'https://github.com', runId: 42, + }, + core: { info() {}, setFailed: message => failures.push(message) }, + }); + return failures; +} + +for (const permission of ['write', 'maintain', 'admin']) { + test(`${permission} access allows an issue-less PR`, async () => { + assert.deepEqual(await check({ permission }), []); + }); +} + +for (const body of ['', 'No issue required: typo fix', '']) { + test(`community PR without linked issues fails for body ${JSON.stringify(body)}`, async () => { + const failures = await check({ body }); + assert.equal(failures.length, 1); + assert.match(failures[0], /must link an issue with state:accepted/); + }); +} + +test('all linked issues must be accepted, regardless of PR body or triggering actor', async () => { + const failures = await check({ + body: 'No issue required: small fix', + issues: [{ number: 1, labels: [{ name: 'state:accepted' }] }, { number: 2 }], + }); + assert.equal(failures.length, 1); + assert.match(failures[0], /issues\/2/); +}); + +test('accepted issues in the target repository pass', async () => { + assert.deepEqual(await check({ issues: [{ number: 1, labels: [{ name: 'state:accepted' }] }] }), []); +}); + +test('accepted issues in another repository cannot bypass the gate', async () => { + const failures = await check({ issues: [{ number: 1, owner: 'contributor', labels: [{ name: 'state:accepted' }] }] }); + assert.equal(failures.length, 1); + assert.match(failures[0], /Acceptance must be recorded in NVIDIA\/OpenShell/); +}); + +test('a non-collaborator still needs an accepted issue', async () => { + assert.equal((await check({ permissionError: 404, body: 'No issue required: typo fix' })).length, 1); +}); + +test('permission lookup failures do not grant an exemption', async () => { + await assert.rejects(check({ permissionError: 403 }), /lookup failed/); +}); + +for (const [option, message] of [ + ['pullError', /PR lookup failed/], + ['issueError', /Issue lookup failed/], + ['labelError', /Label lookup failed/], +]) { + test(`${option} fails the workflow invocation`, async () => { + await assert.rejects(check({ [option]: true, issues: [{ number: 1 }] }), message); + }); +} + +test('PRs sharing a head SHA evaluate their own issue references in either order', async () => { + const rejected = { number: 7, headSha: 'shared-head', issues: [{ number: 1 }] }; + const accepted = { + number: 8, headSha: 'shared-head', + issues: [{ number: 2, labels: [{ name: 'state:accepted' }] }], + }; + for (const pair of [[rejected, accepted], [accepted, rejected]]) { + const results = await Promise.all(pair.map(input => check(input))); + pair.forEach((input, index) => { + assert.equal(results[index].length, input.number === 7 ? 1 : 0); + }); + } +}); + +test('a failed refresh rejects after an earlier successful evaluation', async () => { + assert.deepEqual(await check({ permission: 'write' }), []); + await assert.rejects(check({ permission: 'write', pullError: true }), /PR lookup failed/); +});