-
Notifications
You must be signed in to change notification settings - Fork 669
stack 1/5: add contribution intake firewall #900
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
bbe6887
ed09e07
4a59642
4d8a9d0
9e9db37
78817c5
7684973
df2b934
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,35 @@ | ||
| ## Summary | ||
|
|
||
| - Explain the user-visible or maintainer-facing change. | ||
| Explain the user-visible or maintainer-facing change and why this approach is appropriate. | ||
|
|
||
| ## Linked issue | ||
|
|
||
| Closes #<!-- issue number --> | ||
|
|
||
| Implementation pull requests must reference an issue labeled `approved-for-work`. Documentation-only and maintainer-owned integration changes are exempt. | ||
|
|
||
| ## Verification | ||
|
|
||
| - List the commands or checks you ran. | ||
| List the exact commands or checks you ran and their results. Do not write only "tested" or "CI". | ||
|
|
||
| ```text | ||
| bun run typecheck | ||
| bun run test | ||
| ``` | ||
|
|
||
| ## Regression coverage | ||
|
|
||
| Name the test that fails without this change and passes with it. If automated coverage is genuinely impossible, explain why and describe the manual evidence. | ||
|
|
||
| ## Screenshots or recordings | ||
|
|
||
| Required for user-visible dashboard changes. Remove this section when it does not apply. | ||
|
|
||
| ## Checklist | ||
| ## Author responsibility | ||
|
|
||
| - [ ] Scope stays focused and avoids unrelated cleanup. | ||
| - [ ] Docs or release notes were updated when needed. | ||
| - [ ] Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. | ||
| - [ ] I reviewed every changed line and can explain the implementation. | ||
| - [ ] I ran the validation commands listed above. | ||
| - [ ] Behavior changes include focused regression coverage, or I explained why automated coverage is impossible. | ||
| - [ ] The pull request contains no unrelated cleanup, generated churn, or accidental lockfile changes. | ||
| - [ ] I checked automated-review findings critically instead of applying them blindly. | ||
| - [ ] I will remain available to resolve CI failures and review feedback. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| "use strict"; | ||
|
|
||
| const IMPLEMENTATION_PREFIXES = [ | ||
| "src/", | ||
| "gui/", | ||
| "scripts/", | ||
| "tests/", | ||
| "bin/", | ||
| "packages/", | ||
| ]; | ||
|
|
||
| const IMPLEMENTATION_FILES = new Set([ | ||
| "package.json", | ||
| "bun.lock", | ||
| "bunfig.toml", | ||
| "tsconfig.json", | ||
| ]); | ||
|
Comment on lines
+12
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
An external PR that changes only Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [shipping-github] Fixed in |
||
|
|
||
| const REQUIRED_ATTESTATIONS = [ | ||
| "I reviewed every changed line and can explain the implementation.", | ||
| "I ran the validation commands listed above.", | ||
| "Behavior changes include focused regression coverage, or I explained why automated coverage is impossible.", | ||
| "The pull request contains no unrelated cleanup, generated churn, or accidental lockfile changes.", | ||
| "I checked automated-review findings critically instead of applying them blindly.", | ||
| "I will remain available to resolve CI failures and review feedback.", | ||
| ]; | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| function normalizeCheckboxLabel(value) { | ||
| return value.trim().replace(/\s+/g, " "); | ||
| } | ||
|
|
||
| function checkedAttestations(body) { | ||
| const checked = new Set(); | ||
| const text = typeof body === "string" ? body : ""; | ||
| for (const match of text.matchAll(/^\s*[-*+]\s+\[[xX]\]\s+(.+?)\s*$/gm)) { | ||
| checked.add(normalizeCheckboxLabel(match[1])); | ||
| } | ||
| return checked; | ||
| } | ||
|
|
||
| function missingAttestations(body) { | ||
| const checked = checkedAttestations(body); | ||
| return REQUIRED_ATTESTATIONS.filter((label) => !checked.has(label)); | ||
| } | ||
|
|
||
| function extractLinkedIssueNumbers(body) { | ||
| const text = typeof body === "string" ? body : ""; | ||
| const numbers = new Set(); | ||
|
|
||
| for (const match of text.matchAll( | ||
| /\b(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?|refs?|issue)\s*:?\s*#(\d+)\b/gi, | ||
| )) { | ||
| numbers.add(Number(match[1])); | ||
| } | ||
|
|
||
| return [...numbers]; | ||
| } | ||
|
|
||
| function isImplementationPath(path) { | ||
| if (IMPLEMENTATION_FILES.has(path)) return true; | ||
| return IMPLEMENTATION_PREFIXES.some((prefix) => path.startsWith(prefix)); | ||
| } | ||
|
|
||
| function needsApprovedIssue(changedFiles) { | ||
| return changedFiles.some(isImplementationPath); | ||
| } | ||
|
|
||
| function issueIsApproved(issue) { | ||
| if (issue.state !== "open") return false; | ||
| return issue.labels.some((label) => { | ||
| const name = typeof label === "string" ? label : label?.name; | ||
| return name === "approved-for-work"; | ||
|
Comment on lines
+68
to
+72
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
For an external implementation PR, this predicate accepts any issue that still carries Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [shipping-github] Fixed in |
||
| }); | ||
| } | ||
|
|
||
| function assessAdmission({ | ||
| body, | ||
| changedFiles, | ||
| linkedIssues, | ||
| authorHasPushPermission = false, | ||
| }) { | ||
| const failures = []; | ||
| const missing = missingAttestations(body); | ||
|
|
||
| if (missing.length > 0) { | ||
| failures.push({ code: "missing_attestations", missing }); | ||
| } | ||
|
|
||
| if (needsApprovedIssue(changedFiles) && !authorHasPushPermission) { | ||
| if (linkedIssues.length === 0) { | ||
| failures.push({ code: "missing_issue" }); | ||
| } else if (!linkedIssues.some(issueIsApproved)) { | ||
| failures.push({ | ||
| code: "issue_not_approved", | ||
| issues: linkedIssues.map((issue) => issue.number), | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| return failures; | ||
| } | ||
|
|
||
| module.exports = { | ||
| REQUIRED_ATTESTATIONS, | ||
| assessAdmission, | ||
| checkedAttestations, | ||
| extractLinkedIssueNumbers, | ||
| isImplementationPath, | ||
| issueIsApproved, | ||
| missingAttestations, | ||
| needsApprovedIssue, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,162 @@ | ||
| "use strict"; | ||
|
|
||
| const fs = require("node:fs"); | ||
| const path = require("node:path"); | ||
| const { describe, it } = require("node:test"); | ||
| const assert = require("node:assert/strict"); | ||
| const { | ||
| REQUIRED_ATTESTATIONS, | ||
| assessAdmission, | ||
| extractLinkedIssueNumbers, | ||
| missingAttestations, | ||
| needsApprovedIssue, | ||
| } = require("./pr-admission.cjs"); | ||
|
|
||
| function escapeRegExp(value) { | ||
| return value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); | ||
| } | ||
|
|
||
| function completeBody() { | ||
| return [ | ||
| "## Summary", | ||
| "A complete explanation of the change and why it is needed.", | ||
| "", | ||
| "## Linked issue", | ||
| "Closes #123", | ||
| "", | ||
| "## Author responsibility", | ||
| ...REQUIRED_ATTESTATIONS.map((label) => `- [x] ${label}`), | ||
| "", | ||
| ].join("\n"); | ||
| } | ||
|
|
||
| describe("missingAttestations", () => { | ||
| it("rejects unchecked and missing author responsibility items", () => { | ||
| const body = [ | ||
| `- [x] ${REQUIRED_ATTESTATIONS[0]}`, | ||
| `- [ ] ${REQUIRED_ATTESTATIONS[1]}`, | ||
| ].join("\n"); | ||
|
|
||
| assert.deepEqual( | ||
| missingAttestations(body), | ||
| REQUIRED_ATTESTATIONS.slice(1), | ||
| ); | ||
| }); | ||
|
|
||
| it("accepts every required checked item", () => { | ||
| assert.deepEqual(missingAttestations(completeBody()), []); | ||
| }); | ||
| }); | ||
|
|
||
| describe("extractLinkedIssueNumbers", () => { | ||
| it("recognizes closing and reference syntax without duplicates", () => { | ||
| assert.deepEqual( | ||
| extractLinkedIssueNumbers("Closes #12\nRefs: #34\nFixes #12"), | ||
| [12, 34], | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| describe("needsApprovedIssue", () => { | ||
| it("requires an approved issue for implementation paths", () => { | ||
| assert.equal(needsApprovedIssue(["src/router.ts"]), true); | ||
| assert.equal(needsApprovedIssue(["gui/src/App.tsx"]), true); | ||
| assert.equal(needsApprovedIssue(["package.json"]), true); | ||
| assert.equal(needsApprovedIssue(["bunfig.toml"]), true); | ||
| }); | ||
|
|
||
| it("does not require one for documentation-only changes", () => { | ||
| assert.equal( | ||
| needsApprovedIssue(["README.md", "docs-site/src/content/docs/foo.md"]), | ||
| false, | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| describe("assessAdmission", () => { | ||
| it("rejects implementation PRs with no linked issue", () => { | ||
| const failures = assessAdmission({ | ||
| body: completeBody(), | ||
| changedFiles: ["src/router.ts"], | ||
| linkedIssues: [], | ||
| }); | ||
|
|
||
| assert.deepEqual(failures, [{ code: "missing_issue" }]); | ||
| }); | ||
|
|
||
| it("rejects linked issues that are not approved for work", () => { | ||
| const failures = assessAdmission({ | ||
| body: completeBody(), | ||
| changedFiles: ["src/router.ts"], | ||
| linkedIssues: [{ number: 123, labels: ["bug"], state: "open" }], | ||
| }); | ||
|
|
||
| assert.deepEqual(failures, [ | ||
| { code: "issue_not_approved", issues: [123] }, | ||
| ]); | ||
| }); | ||
|
|
||
| it("accepts an approved implementation issue", () => { | ||
| const failures = assessAdmission({ | ||
| body: completeBody(), | ||
| changedFiles: ["src/router.ts"], | ||
| linkedIssues: [ | ||
| { number: 123, labels: [{ name: "approved-for-work" }], state: "open" }, | ||
| ], | ||
| }); | ||
|
|
||
| assert.deepEqual(failures, []); | ||
| }); | ||
|
|
||
| it("rejects closed issues even when they carry the approval label", () => { | ||
| const failures = assessAdmission({ | ||
| body: completeBody(), | ||
| changedFiles: ["src/router.ts"], | ||
| linkedIssues: [ | ||
| { number: 123, labels: [{ name: "approved-for-work" }], state: "closed" }, | ||
| ], | ||
| }); | ||
|
|
||
| assert.deepEqual(failures, [ | ||
| { code: "issue_not_approved", issues: [123] }, | ||
| ]); | ||
| }); | ||
|
|
||
| it("allows maintainers to perform integration work without an issue", () => { | ||
| const failures = assessAdmission({ | ||
| body: completeBody(), | ||
| changedFiles: ["src/router.ts"], | ||
| linkedIssues: [], | ||
| authorHasPushPermission: true, | ||
| }); | ||
|
|
||
| assert.deepEqual(failures, []); | ||
| }); | ||
|
|
||
| it("still requires maintainer attestations", () => { | ||
| const failures = assessAdmission({ | ||
| body: "", | ||
| changedFiles: ["src/router.ts"], | ||
| linkedIssues: [], | ||
| authorHasPushPermission: true, | ||
| }); | ||
|
|
||
| assert.equal(failures[0].code, "missing_attestations"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("template parity", () => { | ||
| it("keeps the PR template attestations in sync with REQUIRED_ATTESTATIONS", () => { | ||
| const template = fs.readFileSync( | ||
| path.join(__dirname, "..", "PULL_REQUEST_TEMPLATE.md"), | ||
| "utf8", | ||
| ); | ||
| for (const label of REQUIRED_ATTESTATIONS) { | ||
| const pattern = new RegExp( | ||
| `^\\s*[-*+]\\s+\\[[ xX]\\]\\s+${escapeRegExp(label)}\\s*$`, | ||
| "m", | ||
| ); | ||
| assert.match(template, pattern); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This introduces mandatory approved-issue attestations and a five-day automatic-close policy, but neither
CONTRIBUTING.mdnordocs-site/src/content/docs/contributing.mdand its translations describe those requirements. Contributors following the published guide therefore learn the contract only after their PR fails and is placed on the closure timer; update the English contributor documentation and keep translated versions consistent.AGENTS.md reference: AGENTS.md:L200-L201
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[shipping-github] Declined here because it is already implemented later in this stack: #905 adds the
CONTRIBUTING.md"Pull request contract" section, the docs-site pagecontributing/pr-quality.md, andMAINTAINERS.mdpolicy notes coveringapproved-for-work, attestations, and the automatic-close window. Keeping the docs change in #905 avoids duplicating it in #900.