From 04ccb7939ad715df8cddb2918c208c96491636fb Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 3 Aug 2026 12:10:35 +0900 Subject: [PATCH] ci: sponsor the surfaces where a bad merge is expensive, and say so in the docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Consolidates the parts of @Wibias's five-PR governance stack (#900, #902, #905) that reduce risk without adding friction, and drops the parts that do the opposite. The measurement behind the ordering: Windows was the last job to finish in 23 of 23 recent CI runs at a 17m41s median, so #899 was the actual bottleneck and everything here is judged by whether it makes the lane worse. Kept, from #902's trust lane: authentication, credential handling, GitHub Actions workflows, release automation, and dependency installation need a maintainer to sponsor the change before it merges. MAINTAINERS.md already requires security review for exactly these; this makes the requirement visible on the pull request instead of relying on a reviewer noticing. It runs inside the existing hygiene job rather than adding a workflow, and it applies to every contributor — blast radius does not depend on how many PRs someone has merged, which is why the upstream first-timer exemption is gone. Dropped, from the same PR: the 500-line cap and the one-open-PR limit. A provider preset with its registry rows, adapter wiring, tests, and five locales clears 500 lines by itself, and several good first contributions here have. Telling a newcomer their fix is too big is a worse failure than reviewing a large diff. Dropped, from #900: the admission gate requiring a pre-approved issue, and the five-day auto-close. The DeepSeek reasoning replay, the Cursor Grok parameters, the AgentRouter EOF tolerance, the tool-result image forwarding — every one arrived as an unplanned PR from someone who hit the bug. A gate that required a planning discussion first would have lost all of them. Kept, from #905: CODEOWNERS entries for the high-impact runtime directories, and the contributor documentation — rewritten to describe what is actually enforced. The submitted version documented the approved-for-work gate, the size caps, and the automatic closure timers, none of which exist here, and publishing rules the repository does not enforce is worse than publishing none. Not included: #901's readiness gate. It makes CodeRabbit's judgment blocking and triggers per check_run, which scales with the job count #899 just raised. Co-authored-by: Wibias <37517432+Wibias@users.noreply.github.com> --- .github/CODEOWNERS | 6 ++ .github/scripts/pr-sponsored-surface.cjs | 87 +++++++++++++++++++ .github/scripts/pr-sponsored-surface.test.cjs | 85 ++++++++++++++++++ .github/workflows/issue-quality-tests.yml | 5 ++ .github/workflows/pr-hygiene.yml | 19 +++- CONTRIBUTING.md | 24 +++++ .../content/docs/contributing/pr-quality.md | 64 ++++++++++++++ 7 files changed, 289 insertions(+), 1 deletion(-) create mode 100644 .github/scripts/pr-sponsored-surface.cjs create mode 100644 .github/scripts/pr-sponsored-surface.test.cjs create mode 100644 docs-site/src/content/docs/contributing/pr-quality.md diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 297343edf..d4e18ec48 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -1,6 +1,12 @@ # Default reviewers * @lidge-jun @Ingwannu @Wibias +# High-impact runtime behavior +/src/adapters/ @lidge-jun @Ingwannu @Wibias +/src/providers/ @lidge-jun @Ingwannu @Wibias +/src/codex/ @lidge-jun @Ingwannu @Wibias +/src/server/ @lidge-jun @Ingwannu @Wibias + # Repository automation and release security /.github/ @lidge-jun @Ingwannu /scripts/release.ts @lidge-jun @Ingwannu diff --git a/.github/scripts/pr-sponsored-surface.cjs b/.github/scripts/pr-sponsored-surface.cjs new file mode 100644 index 000000000..36b6e400a --- /dev/null +++ b/.github/scripts/pr-sponsored-surface.cjs @@ -0,0 +1,87 @@ +"use strict"; + +/** + * Sponsorship for surfaces where a bad merge is expensive and hard to unwind. + * + * Derived from @Wibias's trust-lane gate (#902), narrowed on purpose. That + * version also capped first-time contributors at one open pull request and 500 + * changed lines. Those caps are not here: a provider preset with its registry + * rows, adapter wiring, tests, and five locales clears 500 lines by itself, and + * several good first contributions to this repository have. Telling a newcomer + * their fix is too big is a worse failure than reviewing a large diff. + * + * What survives is the part that is about blast radius rather than trust: + * authentication, credential handling, GitHub Actions workflows, release + * automation, and dependency installation need a maintainer to sponsor the + * change before it merges. `MAINTAINERS.md` already requires security review for + * exactly these; this makes the requirement visible on the pull request instead + * of relying on a reviewer noticing. + * + * It applies to EVERY contributor, not only first-timers. A maintainer with push + * permission is exempt because their own review is the sponsorship. + */ + +const RESTRICTED_PREFIXES = [ + ".github/workflows/", + "src/oauth/", +]; + +const RESTRICTED_FILES = new Set([ + // Release and packaging automation executed by the release workflow. + "scripts/release.ts", + "scripts/release-notes.ts", + "scripts/prepare-package.ts", + // Authentication, credential, and secret handling. Mirrors the CODEOWNERS + // security boundary. + "src/codex/auth-api.ts", + "src/codex/auth-collision.ts", + "src/codex/auth-context.ts", + "src/cli/account-auth.ts", + "src/cli/status-oauth.ts", + "src/lib/admin-secrets.ts", + "src/lib/service-secrets.ts", + "src/lib/windows-secret-acl.ts", + "src/server/auth-cors.ts", + "src/server/management-api.ts", + "src/server/management-auth.ts", + "src/server/management/oauth-account-routes.ts", + "src/claude/auth-detect.ts", + "src/claude/auth-mode-migration.ts", + "src/claude/auth-mode.ts", + // Dependency surfaces. + "package.json", + "bun.lock", +]); + +function isRestrictedPath(path) { + return RESTRICTED_FILES.has(path) || RESTRICTED_PREFIXES.some((prefix) => path.startsWith(prefix)); +} + +function hasSponsorship(labels) { + return (labels || []).some( + (label) => (typeof label === "string" ? label : label?.name) === "maintainer-sponsored", + ); +} + +/** + * @returns {{ code: string, paths: string[] }[]} empty when the pull request may proceed + */ +function assessSponsoredSurface({ + authorHasPushPermission = false, + changedFiles = [], + labels = [], +}) { + // A maintainer's own change carries its own sponsorship. + if (authorHasPushPermission) return []; + const restricted = changedFiles.filter(isRestrictedPath); + if (restricted.length === 0) return []; + if (hasSponsorship(labels)) return []; + return [{ code: "unsponsored_surface", paths: restricted }]; +} + +module.exports = { + RESTRICTED_FILES, + RESTRICTED_PREFIXES, + assessSponsoredSurface, + isRestrictedPath, +}; diff --git a/.github/scripts/pr-sponsored-surface.test.cjs b/.github/scripts/pr-sponsored-surface.test.cjs new file mode 100644 index 000000000..df5e96d4d --- /dev/null +++ b/.github/scripts/pr-sponsored-surface.test.cjs @@ -0,0 +1,85 @@ +"use strict"; + +const { describe, it } = require("node:test"); +const assert = require("node:assert/strict"); +const { assessSponsoredSurface, isRestrictedPath } = require("./pr-sponsored-surface.cjs"); + +describe("isRestrictedPath", () => { + it("covers auth, workflow, release, and dependency surfaces", () => { + for (const path of [ + "src/oauth/store.ts", + ".github/workflows/release.yml", + "scripts/release.ts", + "src/server/management-auth.ts", + "package.json", + "bun.lock", + ]) { + assert.equal(isRestrictedPath(path), true, path); + } + }); + + it("leaves ordinary product surfaces alone", () => { + for (const path of [ + "src/adapters/anthropic.ts", + "src/providers/registry.ts", + "gui/src/pages/Providers.tsx", + "tests/anthropic.test.ts", + "docs-site/src/content/docs/x.md", + ]) { + assert.equal(isRestrictedPath(path), false, path); + } + }); +}); + +describe("assessSponsoredSurface", () => { + it("requires sponsorship for a restricted surface", () => { + const failures = assessSponsoredSurface({ changedFiles: ["src/oauth/store.ts"] }); + assert.equal(failures[0].code, "unsponsored_surface"); + assert.deepEqual(failures[0].paths, ["src/oauth/store.ts"]); + }); + + it("passes once a maintainer sponsors it", () => { + assert.deepEqual( + assessSponsoredSurface({ + changedFiles: ["src/oauth/store.ts"], + labels: ["maintainer-sponsored"], + }), + [], + ); + assert.deepEqual( + assessSponsoredSurface({ + changedFiles: ["src/oauth/store.ts"], + labels: [{ name: "maintainer-sponsored" }], + }), + [], + ); + }); + + it("exempts an author who can already push", () => { + assert.deepEqual( + assessSponsoredSurface({ + authorHasPushPermission: true, + changedFiles: ["scripts/release.ts"], + }), + [], + ); + }); + + it("applies to every contributor, not only first-timers", () => { + // The upstream trust lane exited early for anyone who was not a first-time + // contributor. Blast radius does not depend on how many PRs someone has + // merged, so this one does not carry that exemption. + const failures = assessSponsoredSurface({ + authorAssociation: "MEMBER", + changedFiles: [".github/workflows/release.yml"], + }); + assert.equal(failures[0].code, "unsponsored_surface"); + }); + + it("ignores a pull request that touches nothing restricted", () => { + assert.deepEqual( + assessSponsoredSurface({ changedFiles: ["src/adapters/anthropic.ts", "tests/a.test.ts"] }), + [], + ); + }); +}); diff --git a/.github/workflows/issue-quality-tests.yml b/.github/workflows/issue-quality-tests.yml index 0d36e0b2a..c8f9810d8 100644 --- a/.github/workflows/issue-quality-tests.yml +++ b/.github/workflows/issue-quality-tests.yml @@ -13,6 +13,8 @@ on: - ".github/scripts/enforce-pr-target.test.cjs" - ".github/scripts/pr-hygiene.cjs" - ".github/scripts/pr-hygiene.test.cjs" + - ".github/scripts/pr-sponsored-surface.cjs" + - ".github/scripts/pr-sponsored-surface.test.cjs" - ".github/scripts/issue-translation.cjs" - ".github/scripts/issue-translation.test.cjs" - ".github/scripts/issue-triage.cjs" @@ -37,6 +39,8 @@ on: - ".github/scripts/enforce-pr-target.test.cjs" - ".github/scripts/pr-hygiene.cjs" - ".github/scripts/pr-hygiene.test.cjs" + - ".github/scripts/pr-sponsored-surface.cjs" + - ".github/scripts/pr-sponsored-surface.test.cjs" - ".github/scripts/issue-translation.cjs" - ".github/scripts/issue-translation.test.cjs" - ".github/scripts/issue-triage.cjs" @@ -70,6 +74,7 @@ jobs: node --test .github/scripts/pr-labeler.test.cjs node --test .github/scripts/enforce-pr-target.test.cjs node --test .github/scripts/pr-hygiene.test.cjs + node --test .github/scripts/pr-sponsored-surface.test.cjs node --test .github/scripts/issue-translation.test.cjs node --test .github/scripts/issue-triage.test.cjs node --test .github/scripts/parse-issue-translation-response.test.cjs diff --git a/.github/workflows/pr-hygiene.yml b/.github/workflows/pr-hygiene.yml index d360a1085..4ead21bdd 100644 --- a/.github/workflows/pr-hygiene.yml +++ b/.github/workflows/pr-hygiene.yml @@ -38,6 +38,9 @@ jobs: const { assessHygiene } = require( path.join(process.cwd(), ".github", "scripts", "pr-hygiene.cjs"), ); + const { assessSponsoredSurface } = require( + path.join(process.cwd(), ".github", "scripts", "pr-sponsored-surface.cjs"), + ); const { owner, repo } = context.repo; const pull_number = context.payload.pull_request.number; @@ -49,6 +52,7 @@ jobs: "suppression-approved": ["5319e7", "Maintainer approved a new type or lint suppression"], "generated-change-approved": ["5319e7", "Maintainer approved committed generated output"], "dependency-change-approved": ["5319e7", "Maintainer approved exceptional dependency or lockfile handling"], + "maintainer-sponsored": ["5319e7", "Maintainer sponsors this change to an auth, workflow, release, or dependency surface"], }; async function ensureLabel(name) { @@ -89,7 +93,19 @@ jobs: } } } - const failures = assessHygiene({ files, labels: [...labels] }); + // Sponsorship is head-independent: it is about which surfaces the + // change touches, not about the state of a particular revision, so + // it is NOT cleared by the synchronize sweep above. + const failures = [ + ...assessHygiene({ files, labels: [...labels] }), + ...assessSponsoredSurface({ + authorHasPushPermission: ["OWNER", "MEMBER", "COLLABORATOR"].includes( + pr.author_association, + ), + changedFiles: files.map((file) => file.filename), + labels: [...labels], + }), + ]; async function setBlocked(blocked) { if (blocked && !labels.has(blockedLabel)) { @@ -130,6 +146,7 @@ jobs: new_suppression: "A new TypeScript, lint, formatter, or similar suppression was added. Fix the underlying issue or obtain `suppression-approved`.", focused_or_skipped_test: "A focused or skipped test was added. Restore the complete suite or obtain `test-exception-approved`.", empty_catch: "An empty catch block was added. Handle, report, or deliberately propagate the error.", + unsponsored_surface: "This changes an authentication, workflow, release-automation, or dependency surface. `MAINTAINERS.md` requires security review for these; ask a maintainer to apply `maintainer-sponsored` once they have reviewed it.", }; const lines = failures.map((failure) => { const paths = failure.paths?.length diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index eb3fdab11..e8d288993 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -3,6 +3,7 @@ Thanks for helping with opencodex. - Start with the canonical guide: [Contributing](https://opencodex.me/contributing/) +- Pull-request quality contract: [Review readiness and author responsibility](https://opencodex.me/contributing/pr-quality/) - Public user docs live in [`docs-site/`](./docs-site) - Current maintainer invariants live in [`structure/`](./structure) - Maintainer roles and merge policy live in [`MAINTAINERS.md`](./MAINTAINERS.md) @@ -31,6 +32,29 @@ Source development requires the `bun` CLI on your `PATH`. The published npm pack Bun runtime for end users, but contributor commands such as `bun install`, `bun run test`, and `bun run prepush` run from your local Bun installation. +## Pull request contract + +A ready-for-review PR is the author's claim that the change is complete, understood, tested, and suitable for merging. Opening a PR does not transfer responsibility for the branch to maintainers. + +- **You do not need permission to fix something.** An unplanned PR for a bug you + hit is welcome, and several of this project's better fixes arrived exactly that + way. Opening an issue first helps for larger or design-shaped work, but it is + not an admission requirement. +- Authors own CI failures, missing tests, merge conflicts, and review fixes. + Maintainers identify problems; they are not required to implement or debug the + fixes for contributors. +- Behavior changes include focused regression tests. Claims such as "tested" or + "CI" without named commands and results are not evidence. The hygiene gate + checks this mechanically, and its failures are deterministic — read the message + and you know what to change. +- Authentication, workflow, release automation, and dependency-installation + surfaces need a maintainer to sponsor the change (`maintainer-sponsored`) + before merge. Those are the places where a bad merge is expensive and hard to + unwind, which is why they are the only pre-approved surfaces here. +- A PR that stalls with unresolved review feedback may be closed, with the reason + stated. A closed PR can be reopened once the stated reason is resolved, or + replaced with a clean one. + ## Pre-push hook After cloning, run once to install a local pre-push hook that runs the typecheck, diff --git a/docs-site/src/content/docs/contributing/pr-quality.md b/docs-site/src/content/docs/contributing/pr-quality.md new file mode 100644 index 000000000..3f8106ad0 --- /dev/null +++ b/docs-site/src/content/docs/contributing/pr-quality.md @@ -0,0 +1,64 @@ +--- +title: Pull request quality contract +description: Review readiness, contributor responsibility, trust lanes, and closure policy for OpenCodex pull requests. +--- + +## You do not need permission to fix something + +An unplanned pull request for a bug you actually hit is welcome. Several of this +project's better fixes arrived exactly that way — a routed model stalling after +tool calls, a provider sending the wrong model parameters, images being flattened +out of tool results. None of those started from a planning discussion, and a +gate that required one would have lost all of them. + +Opening an issue first genuinely helps for larger or design-shaped work, where +agreeing on the approach saves you from building the wrong thing. That is advice, +not an admission requirement. + +## What a ready pull request claims + +Marking a PR ready for review is a claim that the change is complete, understood, +and tested. Opening it does not transfer responsibility for the branch to the +maintainers. + +Authors are expected to understand every changed line, name the exact commands +and results behind any validation claim, add focused regression coverage for +behavior changes, and stay available to resolve CI and review feedback. +Maintainers identify problems; they are not expected to repair contributor +branches, write the missing tests, or translate automated findings into patches +on your behalf. + +"Tested" or "CI passes" without named commands and results is not evidence. + +## Automated gates + +Two checks run before human review, and both are deterministic — the failure +message tells you exactly what to change: + +- **Hygiene.** Behavior changes need a test; new lint or type suppressions, + focused or skipped tests, empty catch blocks, edited generated output, and a + lockfile changed without its manifest each need an explicit approval label. + A comment-only change to a source file is not a behavior change and owes no + test. +- **Cross-platform CI.** The suite runs sharded on Linux and in full on macOS for + every pull request. Windows runs at the shipping boundary — on promotion to + `main` or `preview` — so a slow or flaky Windows runner cannot decide when your + pull request turns green. + +CodeRabbit reviews every PR and its findings are advisory. Address what it gets +right; say why when it is wrong. It does not block a merge. + +## Sponsored surfaces + +Authentication, credential handling, GitHub Actions workflows, release +automation, and dependency installation need a maintainer to sponsor the change +(`maintainer-sponsored`) before it merges. A bad merge on those surfaces is +expensive and hard to unwind, which is why they are the only surfaces gated this +way. Everything else is open. + +## When a pull request is closed + +A PR that stalls with unresolved review feedback may be closed, with the reason +stated plainly. Closure is not a verdict on the contributor: reopen it once the +stated reason is resolved, or replace it with a clean one. Ask if the reason is +not clear.