Skip to content
Closed
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 28 additions & 6 deletions .github/PULL_REQUEST_TEMPLATE.md
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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Document the new intake contract in contributor guides

This introduces mandatory approved-issue attestations and a five-day automatic-close policy, but neither CONTRIBUTING.md nor docs-site/src/content/docs/contributing.md and 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 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

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 page contributing/pr-quality.md, and MAINTAINERS.md policy notes covering approved-for-work, attestations, and the automatic-close window. Keeping the docs change in #905 avoids duplicating it in #900.


## 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.
112 changes: 112 additions & 0 deletions .github/scripts/pr-admission.cjs
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Treat bunfig.toml as an implementation file

An external PR that changes only bunfig.toml is classified as documentation/policy work and does not need an approved issue, even though this repository uses that file to set the Bun test root and preload the real-home write guard. Removing or changing those settings materially alters test execution and safety, so add bunfig.toml to the implementation-file set.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[shipping-github] Fixed in 4d8a9d04: bunfig.toml is now in IMPLEMENTATION_FILES (it sets the Bun test root and preloads the real-home write guard), with a classification test and a matching design-doc scope update.


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.",
];
Comment thread
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject closed issues when checking work approval

For an external implementation PR, this predicate accepts any issue that still carries approved-for-work, even if that issue was already closed after another implementation landed. Because GitHub does not automatically remove labels on close, contributors can repeatedly reference an old completed issue and bypass the agreed-scope intake gate; carry the issue state into the validator and require the approved issue to remain open.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[shipping-github] Fixed in 4d8a9d04: issueIsApproved now requires state === "open", the workflow carries issue.state into the validator, and a regression test covers an approved-but-closed issue. The design record now says "open issue".

});
}

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,
};
162 changes: 162 additions & 0 deletions .github/scripts/pr-admission.test.cjs
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);
}
});
});
Loading
Loading