ci: sponsor high-blast-radius surfaces, consolidate the governance stack #920
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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", | ||
| ); | ||
|
Comment on lines
+60
to
+63
There was a problem hiding this comment.
AGENTS.md reference: .github/AGENTS.md:L5-L8 Useful? React with 👍 / 👎. |
||
| } | ||
|
|
||
| /** | ||
| * @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 []; | ||
|
There was a problem hiding this comment.
Returning here exempts every push-capable author's PR before its restricted paths are inspected, treating the author's own review as sponsorship. AGENTS.md reference: .github/AGENTS.md:L5-L8 Useful? React with 👍 / 👎. |
||
| 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, | ||
| }; | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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"] }), | ||
| [], | ||
| ); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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: | |
| } | ||
| } | ||
| } | ||
| // 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. | ||
|
Comment on lines
+96
to
+98
There was a problem hiding this comment.
When a sponsored PR receives a AGENTS.md reference: .github/AGENTS.md:L5-L8 Useful? React with 👍 / 👎. |
||
| const failures = [ | ||
| ...assessHygiene({ files, labels: [...labels] }), | ||
| ...assessSponsoredSurface({ | ||
| authorHasPushPermission: ["OWNER", "MEMBER", "COLLABORATOR"].includes( | ||
| pr.author_association, | ||
| ), | ||
| changedFiles: files.map((file) => file.filename), | ||
|
There was a problem hiding this comment.
The AGENTS.md reference: AGENTS.md:L199-L205 Useful? React with 👍 / 👎. |
||
| 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 | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. | ||
|
Comment on lines
+43
to
+46
There was a problem hiding this comment.
This new page says Windows runs only at the shipping boundary, while AGENTS.md reference: docs-site/AGENTS.md:L7-L10 Useful? React with 👍 / 👎. |
||
|
|
||
| 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. | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 allowlist does not cover the credential-handling scope advertised by the check and documentation: for example,
src/providers/api-keys.tspersists provider secrets andsrc/codex/account-store.tspersists and refreshes access/refresh tokens, yet neither matches these prefixes nor appears inRESTRICTED_FILES. A contributor can change either file without sponsorship, so the classifier needs to include the actual credential stores and handlers rather than only selected auth filenames.AGENTS.md reference: AGENTS.md:L199-L205
Useful? React with 👍 / 👎.