-
Notifications
You must be signed in to change notification settings - Fork 670
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 1 commit
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,100 @@ | ||
| "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/auth/", | ||
| "src/oauth/", | ||
| ]; | ||
| const RESTRICTED_FILES = new Set([ | ||
| "scripts/release.ts", | ||
| "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 👍 / 👎.
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 IMPLEMENTATION_PREFIXES = ["src/", "gui/", "scripts/", "tests/", "bin/", "packages/", ".github/workflows/"]; | ||
| const IMPLEMENTATION_FILES = new Set(["package.json", "bun.lock", "tsconfig.json"]); | ||
|
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.
A first-time contributor changing only AGENTS.md reference: .github/AGENTS.md:L7-L8 Useful? React with 👍 / 👎. |
||
|
|
||
| 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.labels || []).some((label) => | ||
| (typeof label === "string" ? label : label?.name) === labelName, | ||
| ), | ||
| ); | ||
| } | ||
|
|
||
| function assessTrustLane({ | ||
| authorAssociation, | ||
| authorHasPushPermission = false, | ||
| files = [], | ||
| linkedIssues = [], | ||
| otherOpenImplementationPrs = [], | ||
| }) { | ||
| if (authorHasPushPermission || !isFirstTimeContributor(authorAssociation)) return []; | ||
| if (!files.some((file) => isImplementationPath(file.filename))) return []; | ||
|
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.
Path classification examines only AGENTS.md reference: .github/AGENTS.md:L7-L8 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] Already fixed in |
||
|
|
||
| const failures = []; | ||
| if (otherOpenImplementationPrs.length > 0) { | ||
| failures.push({ | ||
| code: "active_pr_limit", | ||
| pullRequests: otherOpenImplementationPrs, | ||
| }); | ||
|
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 predicate rejects a PR whenever any other implementation PR by the author is open, so after a second PR is blocked, the next synchronize or edit event on the original PR makes it see the second and become blocked too. Concurrently opened PRs can likewise both fail immediately, leaving zero active submissions instead of one. Select a deterministic keeper, such as the oldest open implementation PR, and reject only the others. 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 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 = files | ||
| .map((file) => file.filename) | ||
| .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,79 @@ | ||
| "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 👍 / 👎.
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. |
||
|
|
||
| const { describe, it } = require("node:test"); | ||
| const assert = require("node:assert/strict"); | ||
| const { | ||
| MAX_FIRST_TIME_CHANGED_LINES, | ||
| assessTrustLane, | ||
| isFirstTimeContributor, | ||
| 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); | ||
| }); | ||
| }); | ||
|
|
||
| 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: [812], | ||
| }); | ||
| assert.deepEqual(failures[0], { code: "active_pr_limit", pullRequests: [812] }); | ||
| }); | ||
|
|
||
| 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" }] }], | ||
| }); | ||
| assert.deepEqual(failures, []); | ||
| }); | ||
|
|
||
| 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"] }], | ||
| }); | ||
| assert.deepEqual(failures, []); | ||
| }); | ||
|
|
||
| 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 }] }), []); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,172 @@ | ||
| name: PR trust lane | ||
|
|
||
| on: | ||
| pull_request_target: | ||
| types: [opened, reopened, edited, synchronize, ready_for_review] | ||
|
Comment on lines
+3
to
+5
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.
This workflow listens only for changes to the currently evaluated PR. When a maintainer adds AGENTS.md reference: .github/AGENTS.md:L23-L24 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] Already addressed in |
||
|
|
||
| # Trusted default-branch script only; no PR-head checkout or execution. | ||
| permissions: | ||
| contents: read | ||
| issues: write | ||
| pull-requests: write | ||
|
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.
This AGENTS.md reference: .github/AGENTS.md:L12-L17 Useful? React with 👍 / 👎. |
||
|
|
||
| concurrency: | ||
| group: pr-trust-lane-${{ github.event.pull_request.number }} | ||
| cancel-in-progress: true | ||
|
|
||
| jobs: | ||
| trust-lane: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout trusted trust-lane script | ||
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | ||
| with: | ||
| ref: ${{ github.event.repository.default_branch }} | ||
| persist-credentials: false | ||
| sparse-checkout: .github/scripts | ||
|
|
||
| - name: Enforce first-contribution limits | ||
| uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 | ||
| with: | ||
| script: | | ||
| const path = require("node:path"); | ||
| const { extractLinkedIssueNumbers } = require( | ||
| path.join(process.cwd(), ".github", "scripts", "pr-admission.cjs"), | ||
| ); | ||
| const { | ||
| assessTrustLane, | ||
| isImplementationPath, | ||
| } = require( | ||
| path.join(process.cwd(), ".github", "scripts", "pr-trust-lane.cjs"), | ||
| ); | ||
|
|
||
| const { owner, repo } = context.repo; | ||
| const pull_number = context.payload.pull_request.number; | ||
| const marker = "<!-- pr-trust-lane -->"; | ||
| const blockedLabel = "intake: trust-lane-blocked"; | ||
|
Comment on lines
+59
to
+60
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.
When this workflow finds a violation it only adds Useful? React with 👍 / 👎. |
||
|
|
||
| const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number }); | ||
| const files = await github.paginate(github.rest.pulls.listFiles, { | ||
| owner, repo, pull_number, per_page: 100, | ||
| }); | ||
|
|
||
| let permission = "read"; | ||
| try { | ||
| const response = await github.rest.repos.getCollaboratorPermissionLevel({ | ||
| owner, repo, username: pr.user.login, | ||
| }); | ||
| permission = response.data.permission; | ||
| } catch (error) { | ||
| core.warning(`Permission lookup failed: ${error.message}`); | ||
| } | ||
| const authorHasPushPermission = ["admin", "maintain", "write"].includes(permission); | ||
|
|
||
| const linkedIssues = []; | ||
| for (const issue_number of extractLinkedIssueNumbers(pr.body)) { | ||
| try { | ||
| const { data: issue } = await github.rest.issues.get({ owner, repo, issue_number }); | ||
| if (!issue.pull_request) linkedIssues.push({ number: issue.number, labels: issue.labels }); | ||
|
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.
Because AGENTS.md reference: .github/AGENTS.md:L16-L17 Useful? React with 👍 / 👎. |
||
| } catch (error) { | ||
| core.warning(`Could not load issue #${issue_number}: ${error.message}`); | ||
| } | ||
| } | ||
|
|
||
| const open = await github.paginate(github.rest.pulls.list, { | ||
| owner, repo, state: "open", per_page: 100, | ||
| }); | ||
| const otherOpenImplementationPrs = []; | ||
| for (const candidate of open) { | ||
| if (candidate.number === pull_number || candidate.user?.login !== pr.user.login) continue; | ||
| const candidateFiles = await github.paginate(github.rest.pulls.listFiles, { | ||
| owner, repo, pull_number candidate.number, per_page: 100, | ||
|
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.
On every triggering event, AGENTS.md reference: .github/AGENTS.md:L23-L25 Useful? React with 👍 / 👎. |
||
| }); | ||
| if (candidateFiles.some((file) => isImplementationPath(file.filename))) { | ||
| otherOpenImplementationPrs.push(candidate.number); | ||
| } | ||
| } | ||
|
|
||
| const failures = assessTrustLane({ | ||
| authorAssociation: pr.author_association, | ||
| authorHasPushPermission, | ||
| files, | ||
| linkedIssues, | ||
| otherOpenImplementationPrs, | ||
| }); | ||
|
|
||
| async function ensureLabel() { | ||
| try { | ||
| await github.rest.issues.getLabel({ owner, repo, name: blockedLabel }); | ||
| } catch (error) { | ||
| if (error.status !== 404) throw error; | ||
| try { | ||
| await github.rest.issues.createLabel({ | ||
| owner, | ||
| repo, | ||
| name: blockedLabel, | ||
| color: "b60205", | ||
| description: "First-contribution limits require maintainer approval", | ||
| }); | ||
| } catch (createError) { | ||
| if (createError.status !== 422) throw createError; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| async function setBlocked(blocked) { | ||
| const labels = new Set(pr.labels.map((label) => label.name)); | ||
| if (blocked && !labels.has(blockedLabel)) { | ||
| await ensureLabel(); | ||
| await github.rest.issues.addLabels({ | ||
| owner, repo, issue_number: pull_number, labels: [blockedLabel], | ||
| }); | ||
| } else if (!blocked && labels.has(blockedLabel)) { | ||
| await github.rest.issues.removeLabel({ | ||
| owner, repo, issue_number pull_number, name: blockedLabel, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| async function upsert(body) { | ||
| const comments = await github.paginate(github.rest.issues.listComments, { | ||
| owner, repo, issue_number: pull_number, per_page: 100, | ||
| }); | ||
| const existing = comments.find( | ||
| (comment) => comment.user?.login === "github-actions[bot]" && comment.body?.includes(marker), | ||
| ); | ||
| if (existing) { | ||
| await github.rest.issues.updateComment({ owner, repo, comment_id: existing.id, body }); | ||
| } else { | ||
| await github.rest.issues.createComment({ owner, repo, issue_number: pull_number, body }); | ||
| } | ||
| } | ||
|
|
||
| if (failures.length === 0) { | ||
| await setBlocked(false); | ||
| await upsert(`${marker}\n\n✅ **Contributor trust-lane requirements passed.**`); | ||
| return; | ||
| } | ||
|
|
||
| await setBlocked(true); | ||
| const lines = failures.flatMap((failure) => { | ||
| if (failure.code === "active_pr_limit") { | ||
| return [ | ||
| "### One active implementation PR", | ||
| `Close or finish ${failure.pullRequests.map((n) => `#${n}`).join(", ")} before opening another implementation PR.`, | ||
| "", | ||
| ]; | ||
| } | ||
| if (failure.code === "first_pr_too_large") { | ||
| return [ | ||
| "### First contribution is too large", | ||
| `This PR changes ${failure.changedLines} lines; the first-contribution ceiling is ${failure.maximum}. Split it, or obtain \`large-change-approved\` on the linked issue before implementation.`, | ||
| "", | ||
| ]; | ||
| } | ||
| return [ | ||
| "### Maintainer sponsorship required", | ||
| `Restricted paths: ${failure.paths.map((p) => `\`${p}\``.),.join(", ")}. The linked issue needs \`maintainer-sponsored\` before a first-time contributor changes these surfaces.`, | ||
| "", | ||
| ]; | ||
| }); | ||
| await upsert([marker, "", "⚠️ **First-contribution limits blocked this PR.**", "", ...lines].join("\n")); | ||
| core.setFailed(`Trust lane failed: ${failures.map((f) => f.code).join(", ")}`); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| # New-contributor trust lane — Design | ||
|
|
||
| **Stack:** 3/5, based on `agent/pr-readiness-gate` | ||
|
|
||
| First-time contributors get a deliberately narrow lane until the repository has evidence that they can scope, validate, and maintain their submissions. | ||
|
|
||
| - One active implementation PR per first-time author. | ||
| - Maximum 500 changed lines for a first implementation PR unless the linked issue has `large-change-approved`. | ||
| - Workflow, OAuth/authentication, release, and dependency surfaces require `maintainer-sponsored` on the linked issue. | ||
| - Documentation-only work, established contributors, and repository collaborators are exempt. | ||
|
Comment on lines
+7
to
+10
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.
A repository-wide search of AGENTS.md reference: AGENTS.md:L200-L201 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] Declined here because it is already implemented later in this stack: #905's |
||
|
|
||
| The workflow uses PR metadata and GitHub APIs only. It does not inspect whether code was written by AI and does not execute untrusted code. | ||
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.