-
Notifications
You must be signed in to change notification settings - Fork 704
stack 3/5: add first-contributor trust lane #902
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
cb508a6
1b5092e
1028cec
842cd51
9361bd3
8a1ebcf
fc6a855
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 |
|---|---|---|
| @@ -0,0 +1,134 @@ | ||
| "use strict"; | ||
|
|
||
| const FIRST_TIME_ASSOCIATIONS = new Set([ | ||
| "FIRST_TIMER", | ||
| "FIRST_TIME_CONTRIBUTOR", | ||
| "NONE", | ||
| ]); | ||
| const MAX_FIRST_TIME_CHANGED_LINES = 500; | ||
| 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. This mirrors the | ||
| // CODEOWNERS security boundary; `src/auth/` does not exist in this repository. | ||
| "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", | ||
| ]); | ||
|
Comment on lines
+13
to
+38
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.
The exact-name set covers only root AGENTS.md reference: AGENTS.md:L187-L193 Useful? React with 👍 / 👎.
Comment on lines
+13
to
+38
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.
Only AGENTS.md reference: AGENTS.md:L187-L193 Useful? React with 👍 / 👎.
Contributor
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 IMPLEMENTATION_PREFIXES = ["src/", "gui/", "scripts/", "tests/", "bin/", "packages/", ".github/workflows/"]; | ||
| const IMPLEMENTATION_FILES = new Set(["package.json", "bun.lock", "bunfig.toml", "tsconfig.json"]); | ||
|
|
||
| function isFirstTimeContributor(authorAssociation) { | ||
| return FIRST_TIME_ASSOCIATIONS.has(String(authorAssociation || "").toUpperCase()); | ||
| } | ||
|
|
||
| function isImplementationPath(path) { | ||
| return IMPLEMENTATION_FILES.has(path) || IMPLEMENTATION_PREFIXES.some((prefix) => path.startsWith(prefix)); | ||
| } | ||
|
|
||
| function isRestrictedPath(path) { | ||
| return RESTRICTED_FILES.has(path) || RESTRICTED_PREFIXES.some((prefix) => path.startsWith(prefix)); | ||
| } | ||
|
|
||
| function changedLines(files) { | ||
| return (files || []).reduce( | ||
| (total, file) => total + Number(file.additions || 0) + Number(file.deletions || 0), | ||
| 0, | ||
| ); | ||
| } | ||
|
|
||
| function linkedIssueHasLabel(linkedIssues, labelName) { | ||
| return (linkedIssues || []).some((issue) => | ||
| issue.state === "open" && | ||
| (issue.labels || []).some((label) => | ||
| (typeof label === "string" ? label : label?.name) === labelName, | ||
| ), | ||
| ); | ||
| } | ||
|
|
||
| function assessTrustLane({ | ||
| authorAssociation, | ||
| authorHasPushPermission = false, | ||
| files = [], | ||
| changedFiles, | ||
| linkedIssues = [], | ||
| otherOpenImplementationPrs = [], | ||
| currentPr = {}, | ||
| }) { | ||
| if (authorHasPushPermission || !isFirstTimeContributor(authorAssociation)) return []; | ||
| const paths = changedFiles ?? (files || []).map((file) => file.filename); | ||
| if (!paths.some(isImplementationPath)) return []; | ||
|
|
||
| const failures = []; | ||
| // Keep the oldest open implementation PR eligible and reject only newer | ||
| // ones, so a second PR can never block the author's first submission. | ||
| const candidates = [ | ||
| ...(otherOpenImplementationPrs || []).map((pr) => ({ | ||
| number: typeof pr === "number" ? pr : pr.number, | ||
| created_at: typeof pr === "number" ? "" : pr.created_at || "", | ||
| })), | ||
| { number: currentPr.number, created_at: currentPr.created_at || "" }, | ||
| ].filter((pr) => Number.isInteger(pr.number)); | ||
| candidates.sort((a, b) => | ||
| a.created_at < b.created_at ? -1 : a.created_at > b.created_at ? 1 : a.number - b.number, | ||
| ); | ||
| if (candidates.length > 1 && candidates[0].number !== currentPr.number) { | ||
| failures.push({ | ||
| code: "active_pr_limit", | ||
| pullRequests: [candidates[0].number], | ||
| }); | ||
| } | ||
|
|
||
| const size = changedLines(files); | ||
| if ( | ||
| size > MAX_FIRST_TIME_CHANGED_LINES && | ||
| !linkedIssueHasLabel(linkedIssues, "large-change-approved") | ||
| ) { | ||
| failures.push({ | ||
| code: "first_pr_too_large", | ||
| changedLines: size, | ||
| maximum: MAX_FIRST_TIME_CHANGED_LINES, | ||
| }); | ||
| } | ||
|
|
||
| const restricted = paths.filter(isRestrictedPath); | ||
| if ( | ||
| restricted.length > 0 && | ||
| !linkedIssueHasLabel(linkedIssues, "maintainer-sponsored") | ||
| ) { | ||
| failures.push({ code: "restricted_surface", paths: restricted }); | ||
| } | ||
|
|
||
| return failures; | ||
| } | ||
|
|
||
| module.exports = { | ||
| MAX_FIRST_TIME_CHANGED_LINES, | ||
| assessTrustLane, | ||
| changedLines, | ||
| isFirstTimeContributor, | ||
| isImplementationPath, | ||
| isRestrictedPath, | ||
| linkedIssueHasLabel, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,136 @@ | ||
| "use strict"; | ||
|
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.
The new Useful? React with 👍 / 👎.
Contributor
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. |
||
|
|
||
| const { describe, it } = require("node:test"); | ||
| const assert = require("node:assert/strict"); | ||
| const { | ||
| MAX_FIRST_TIME_CHANGED_LINES, | ||
| assessTrustLane, | ||
| isFirstTimeContributor, | ||
| isImplementationPath, | ||
| isRestrictedPath, | ||
| } = require("./pr-trust-lane.cjs"); | ||
|
|
||
| describe("first-time classification", () => { | ||
| it("classifies GitHub first-time associations", () => { | ||
| assert.equal(isFirstTimeContributor("FIRST_TIMER"), true); | ||
| assert.equal(isFirstTimeContributor("FIRST_TIME_CONTRIBUTOR"), true); | ||
| assert.equal(isFirstTimeContributor("NONE"), true); | ||
| assert.equal(isFirstTimeContributor("CONTRIBUTOR"), false); | ||
| }); | ||
|
|
||
| it("recognizes restricted security and dependency surfaces", () => { | ||
| assert.equal(isRestrictedPath(".github/workflows/ci.yml"), true); | ||
| assert.equal(isRestrictedPath("src/oauth/provider.ts"), true); | ||
| assert.equal(isRestrictedPath("package.json"), true); | ||
| assert.equal(isRestrictedPath("src/router.ts"), false); | ||
| }); | ||
|
|
||
| it("restricts the repository's real auth and credential paths", () => { | ||
| for (const p of [ | ||
| "src/codex/auth-api.ts", | ||
| "src/codex/auth-context.ts", | ||
| "src/cli/account-auth.ts", | ||
| "src/lib/admin-secrets.ts", | ||
| "src/lib/service-secrets.ts", | ||
| "src/server/auth-cors.ts", | ||
| "src/server/management-auth.ts", | ||
| "src/server/management-api.ts", | ||
| "src/oauth/store.ts", | ||
| ]) { | ||
| assert.equal(isRestrictedPath(p), true, p); | ||
| } | ||
| assert.equal(isRestrictedPath("src/auth/oauth.ts"), false); | ||
| }); | ||
|
|
||
| it("restricts release and packaging scripts", () => { | ||
| assert.equal(isRestrictedPath("scripts/release-notes.ts"), true); | ||
| assert.equal(isRestrictedPath("scripts/prepare-package.ts"), true); | ||
| assert.equal(isRestrictedPath("scripts/test.ts"), false); | ||
| }); | ||
|
|
||
| it("classifies bunfig.toml as an implementation file", () => { | ||
| assert.equal(isImplementationPath("bunfig.toml"), true); | ||
| }); | ||
| }); | ||
|
|
||
| describe("assessTrustLane", () => { | ||
| const smallRuntimeChange = [{ filename: "src/router.ts", additions: 40, deletions: 5 }]; | ||
|
|
||
| it("limits first-time authors to one active implementation PR", () => { | ||
| const failures = assessTrustLane({ | ||
| authorAssociation: "FIRST_TIME_CONTRIBUTOR", | ||
| files: smallRuntimeChange, | ||
| otherOpenImplementationPrs: [{ number: 812, created_at: "2026-07-01T00:00:00Z" }], | ||
| currentPr: { number: 42, created_at: "2026-08-01T00:00:00Z" }, | ||
| }); | ||
| assert.deepEqual(failures[0], { code: "active_pr_limit", pullRequests: [812] }); | ||
| }); | ||
|
|
||
| it("keeps the oldest implementation PR eligible", () => { | ||
| const failures = assessTrustLane({ | ||
| authorAssociation: "FIRST_TIME_CONTRIBUTOR", | ||
| files: smallRuntimeChange, | ||
| otherOpenImplementationPrs: [{ number: 812, created_at: "2026-08-01T00:00:00Z" }], | ||
| currentPr: { number: 42, created_at: "2026-07-01T00:00:00Z" }, | ||
| }); | ||
| assert.deepEqual(failures, []); | ||
| }); | ||
|
|
||
| it("rejects oversized first implementation PRs without approval", () => { | ||
| const failures = assessTrustLane({ | ||
| authorAssociation: "FIRST_TIMER", | ||
| files: [{ filename: "src/router.ts", additions: MAX_FIRST_TIME_CHANGED_LINES + 1, deletions: 0 }], | ||
| }); | ||
| assert.equal(failures[0].code, "first_pr_too_large"); | ||
| }); | ||
|
|
||
| it("allows oversized work when the linked issue approves it", () => { | ||
| const failures = assessTrustLane({ | ||
| authorAssociation: "FIRST_TIMER", | ||
| files: [{ filename: "src/router.ts", additions: 700, deletions: 0 }], | ||
| linkedIssues: [{ labels: [{ name: "large-change-approved" }], state: "open" }], | ||
| }); | ||
| assert.deepEqual(failures, []); | ||
| }); | ||
|
|
||
| it("rejects approval labels on closed issues", () => { | ||
| const failures = assessTrustLane({ | ||
| authorAssociation: "FIRST_TIMER", | ||
| files: [{ filename: "src/router.ts", additions: 700, deletions: 0 }], | ||
| linkedIssues: [{ labels: [{ name: "large-change-approved" }], state: "closed" }], | ||
| }); | ||
| assert.equal(failures[0].code, "first_pr_too_large"); | ||
| }); | ||
|
|
||
| it("requires sponsorship for restricted surfaces", () => { | ||
| const failures = assessTrustLane({ | ||
| authorAssociation: "NONE", | ||
| files: [{ filename: ".github/workflows/ci.yml", additions: 10, deletions: 2 }], | ||
| }); | ||
| assert.equal(failures[0].code, "restricted_surface"); | ||
| }); | ||
|
|
||
| it("allows sponsored restricted work", () => { | ||
| const failures = assessTrustLane({ | ||
| authorAssociation: "NONE", | ||
| files: [{ filename: "src/oauth/provider.ts", additions: 10, deletions: 2 }], | ||
| linkedIssues: [{ labels: ["maintainer-sponsored"], state: "open" }], | ||
| }); | ||
| assert.deepEqual(failures, []); | ||
| }); | ||
|
|
||
| it("classifies renamed sources as restricted implementation paths", () => { | ||
| const failures = assessTrustLane({ | ||
| authorAssociation: "NONE", | ||
| files: [{ filename: "docs/moved.md", additions: 10, deletions: 2 }], | ||
| changedFiles: ["docs/moved.md", "src/oauth/store.ts"], | ||
| }); | ||
| assert.equal(failures[0].code, "restricted_surface"); | ||
| }); | ||
|
|
||
| it("does not restrict established contributors, maintainers, or docs-only PRs", () => { | ||
| assert.deepEqual(assessTrustLane({ authorAssociation: "CONTRIBUTOR", files: smallRuntimeChange }), []); | ||
| assert.deepEqual(assessTrustLane({ authorAssociation: "NONE", authorHasPushPermission: true, files: smallRuntimeChange }), []); | ||
| assert.deepEqual(assessTrustLane({ authorAssociation: "NONE", files: [{ filename: "README.md", additions: 900 }] }), []); | ||
| }); | ||
| }); | ||
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.
The authentication restriction only matches
src/auth/, but that directory does not exist in this tree; authentication and secret handling instead live in files such assrc/codex/auth-api.ts,src/server/management-auth.ts,src/cli/account-auth.ts, andsrc/lib/admin-secrets.ts. A first-time contributor can therefore modify these security-boundary files without sponsorship. Replace the nonexistent-prefix assumption with coverage of the actual authentication and credential modules, backed by representative tests.AGENTS.md reference: AGENTS.md:L187-L193
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] Fixed in
8a1ebcf2: the deadsrc/auth/prefix is replaced with the repository's actual auth/credential/secret module paths (src/codex/auth-api.ts,src/codex/auth-context.ts,src/codex/auth-collision.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-auth.ts,src/server/management-api.ts,src/server/management/oauth-account-routes.ts,src/claude/auth-*.ts), keepingsrc/oauth/and.github/workflows/prefixes. All covered by tests.