diff --git a/.github/workflows/cla.yml b/.github/workflows/cla.yml index 3456a97..3df676a 100644 --- a/.github/workflows/cla.yml +++ b/.github/workflows/cla.yml @@ -186,12 +186,81 @@ jobs: ].join('\n'); } + function writeFailedBody(login) { + return [ + MARKER, + `@${login}, we saw your \`/orbynAgree\`, but we could not record your agreement. Comment \`/orbynAgree\` again to try again.`, + '', + 'If it keeps failing, tell a maintainer. The `CLA` check stays failing until the agreement is recorded.', + ].join('\n'); + } + function agreedBody(login, comment) { return [ MARKER, `Thanks, @${login}. Your agreement to the [Orbyn Individual Contributor Licence Agreement](${CLA_URL}) is recorded${comment ? ` ([comment](${comment.html_url}))` : ''}. It covers all Orbyn repositories that use the agreement, so you will not be asked again.`, ].join('\n'); } + + const WRITE_FAILED = 'Could not record your agreement. Comment /orbynAgree again to try again.'; + + /** + * The single place that decides the `CLA` status. Success only when the person is exempt, + * the record already holds their account id, or the write to the record has just succeeded. + * A failed write is a `failure` (the person must act), a failed read an `error`. + * `comment` says which bot comment to show; `throws` makes the run red. + */ + function decideStatus({ exempt = false, readError, signed = false, agreementFound = false, writeOk = false, writeError }) { + if (exempt) return { state: 'success', description: 'No agreement needed', comment: 'none', throws: false }; + if (readError) return { state: 'error', description: 'Could not check the agreement record.', comment: 'none', throws: true }; + if (signed) return { state: 'success', description: 'Agreed to the Orbyn contributor agreement', comment: 'agreed-if-present', throws: false }; + if (agreementFound && writeError) return { state: 'failure', description: WRITE_FAILED, comment: 'write-failed', throws: true }; + if (agreementFound && writeOk) return { state: 'success', description: 'Agreed to the Orbyn contributor agreement', comment: 'agreed', throws: false }; + if (agreementFound) return { state: 'failure', description: WRITE_FAILED, comment: 'write-failed', throws: true }; + return { state: 'failure', description: 'Comment /orbynAgree once to agree to the Orbyn contributor agreement.', comment: 'ask', throws: false }; + } + + /** + * Run one check. `read()` returns the record, `findAgreementNow()` the author's valid + * `/orbynAgree` comment (or undefined), and `record(agreement)` writes the signature and + * must reject if it did not. Success after an agreement is only possible once `record` + * has resolved. + */ + async function evaluate({ exempt, userId, read, findAgreementNow, record }) { + if (exempt) return { ...decideStatus({ exempt: true }), agreement: undefined, error: undefined }; + let signed = false; + let readError; + let agreement; + let writeOk = false; + let writeError; + try { + signed = isSigned((await read()).list, userId); + } catch (error) { + readError = error; + } + if (!readError && !signed) { + agreement = findAgreementNow(); + if (agreement) { + try { + await record(agreement); + writeOk = true; + } catch (error) { + writeError = error; + } + } + } + const decision = decideStatus({ readError, signed, agreementFound: Boolean(agreement), writeOk, writeError }); + return { ...decision, agreement, error: readError ?? writeError }; + } + + /** The text of the one bot comment for a decision, or null when it should not be touched. */ + function commentFor(kind, { login, agreement, hasOurs }) { + if (kind === 'ask') return askBody(login); + if (kind === 'agreed') return agreedBody(login, agreement); + if (kind === 'agreed-if-present') return hasOurs ? agreedBody(login, agreement) : null; + if (kind === 'write-failed') return writeFailedBody(login); + return null; + } // matcher:end const version = process.env.VERSION; @@ -216,7 +285,8 @@ jobs: }); if (isExempt(pull, allowedIds)) { - await setStatus('success', 'No agreement needed'); + const done = decideStatus({ exempt: true }); + await setStatus(done.state, done.description); return; } @@ -251,43 +321,39 @@ jobs: if (ours.body !== body) return github.rest.issues.updateComment({ owner, repo, comment_id: ours.id, body }); }; - let signed; - let agreement; - try { - signed = isSigned((await read()).list, pull.user.id); - if (!signed) { - agreement = findAgreement(comments, pull); - if (agreement) { - const { data: cla } = await github.rest.repos.getContent({ owner: 'orbyn-dev', repo: '.github', path: 'CLA.md' }); - const entry = buildEntry({ - pull, comment: agreement, repository: `${owner}/${repo}`, version, headSha, - bodySha256: require('crypto').createHash('sha256').update(agreement.body, 'utf8').digest('hex'), - claBlobSha: cla.sha, - recordedAt: new Date().toISOString(), - }); - await recordSignature({ - read, - write: (list, sha) => store.rest.repos.createOrUpdateFileContents({ - owner: sigOwner, repo: sigRepo, path, sha, - message: `Record CLA ${version} agreement by ${pull.user.login} (${owner}/${repo}#${number})`, - content: Buffer.from(JSON.stringify(list, null, 2) + '\n').toString('base64'), - }), - entry, - }); - signed = true; - core.info(`Recorded ${pull.user.login}.`); - } - } - } catch (error) { - await setStatus('error', 'Could not check the agreement record.'); - throw error; - } + const result = await evaluate({ + exempt: false, + userId: pull.user.id, + read, + findAgreementNow: () => findAgreement(comments, pull), + record: async (agreement) => { + const { data: cla } = await github.rest.repos.getContent({ owner: 'orbyn-dev', repo: '.github', path: 'CLA.md' }); + const entry = buildEntry({ + pull, comment: agreement, repository: `${owner}/${repo}`, version, headSha, + bodySha256: require('crypto').createHash('sha256').update(agreement.body, 'utf8').digest('hex'), + claBlobSha: cla.sha, + recordedAt: new Date().toISOString(), + }); + await recordSignature({ + read, + write: (list, sha) => store.rest.repos.createOrUpdateFileContents({ + owner: sigOwner, repo: sigRepo, path, sha, + message: `Record CLA ${version} agreement by ${pull.user.login} (${owner}/${repo}#${number})`, + content: Buffer.from(JSON.stringify(list, null, 2) + '\n').toString('base64'), + }), + entry, + }); + core.info(`Recorded ${pull.user.login}.`); + }, + }); - if (signed) { - await setStatus('success', 'Agreed to the Orbyn contributor agreement'); - if (ours || agreement) await upsert(agreedBody(pull.user.login, agreement)); - return; + await setStatus(result.state, result.description); + const body = commentFor(result.comment, { login: pull.user.login, agreement: result.agreement, hasOurs: Boolean(ours) }); + if (body) { + try { + await upsert(body); + } catch (error) { + core.warning(`Could not update the comment: ${error.message}`); + } } - - await setStatus('failure', 'Comment /orbynAgree once to agree to the Orbyn contributor agreement.'); - await upsert(askBody(pull.user.login)); + if (result.throws) throw result.error; diff --git a/README.md b/README.md index 07ae9f5..2be7e63 100644 --- a/README.md +++ b/README.md @@ -26,6 +26,8 @@ Product repositories call it from `examples/licence-grant-caller.yml`. Exemptions are by numeric account id (`allowed` input) and by `MEMBER`/`OWNER` association. A private organisation member reads as `CONTRIBUTOR`, so they should make their membership public or be listed by id. +The `CLA` status turns green only after the agreement is in the record. If the write fails, the status stays failing ("Could not record your agreement. Comment /orbynAgree again to try again."), the one bot comment says so, and commenting `/orbynAgree` again retries. + Existing contributors with no open pull request can agree by commenting `/orbynAgree` on any of their old pull requests. See [REVIEW.md](REVIEW.md) for the records, the security notes and the exact manual steps. diff --git a/REVIEW.md b/REVIEW.md index 5a9f3ec..51bc2bf 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -35,6 +35,7 @@ - **Base check.** The check is our own short script on `actions/github-script` v9.0.0 (MIT), pinned by commit. The common tool for this, CLA Assistant Lite (`contributor-assistant/github-action`, Apache-2.0), was archived in March 2026, so it isn't used. None of its code is copied. - **Who is checked.** Only the pull request's author. Co-authors listed in commits aren't checked yet. Organisation members and owners, and the account ids in `allowed`, need nothing (status `CLA` = success, "No agreement needed"). `allowed` holds numeric account ids, not logins (a login can be renamed and reused); the default is Anh (63905487), `dependabot[bot]` (49699333) and `github-actions[bot]` (41898282), all checked with the GitHub API. There is no blanket bot exemption: list any other bot by id. GitHub reports a private organisation member as `CONTRIBUTOR`, not `MEMBER`, so such a person should make their membership public or be listed by id. - **The `CLA` status.** The workflow sets a commit status named `CLA` on the pull request's head commit, from `pull_request_target` and from `issue_comment` (created). The run triggered by `/orbynAgree` records the signature and turns the status green, so there is no stale red check and no re-push or re-run. A repository requires the status context `CLA`. +- **Status values.** `success`: no agreement needed, already in the record, or the record write has succeeded (never earlier). `failure` with "Comment /orbynAgree once to agree to the Orbyn contributor agreement.": nothing agreed yet. `failure` with "Could not record your agreement. Comment /orbynAgree again to try again.": the person did comment, but the write to `cla-signatures` failed (conflicts out of retries, a bad or expired token); the bot comment says the same and the run is red. `error` with "Could not check the agreement record.": the record could not be read. A new `/orbynAgree` comment, or any later event on the pull request, tries again from the start. The decision is one pure function covered by unit tests. - **Security.** It runs on `pull_request_target` and `issue_comment`, and never checks out, builds or runs anything from the pull request. It reads event metadata, the author's account id, the head sha and comment bodies, and passes untrusted text only through the matcher functions, never a shell. Only a comment by the pull request's author (compared by account id), not by a bot, never edited, with the command alone on a line, counts. - **Covered repositories.** `orbyn-engine`, `orbyn-planner`, `orbyn-engineering`, `orbyn.dev` and `orbyn-notes` (decision 9). A caller passes the required `agreement-version` input; there is no default, so a repository that forgets it fails instead of recording agreement to the wrong version. - **Not covered.** `orbyn-fullstack-skills` stays MIT with a DCO and has no CLA check. diff --git a/scripts/cla.test.mjs b/scripts/cla.test.mjs index c85a101..ef334e7 100644 --- a/scripts/cla.test.mjs +++ b/scripts/cla.test.mjs @@ -7,8 +7,8 @@ import { readFileSync } from "node:fs"; const workflow = readFileSync(new URL("../.github/workflows/cla.yml", import.meta.url), "utf8"); const block = workflow.match(/\/\/ matcher:start\n([\s\S]*?)\/\/ matcher:end/)[1]; -const { hasAgreeCommand, isAgreement, findAgreement, isExempt, parseIds, parseRecord, buildEntry, isSigned, recordSignature, askBody, agreedBody } = new Function( - `${block}\nreturn { hasAgreeCommand, isAgreement, findAgreement, isExempt, parseIds, parseRecord, buildEntry, isSigned, recordSignature, askBody, agreedBody };`, +const { decideStatus, evaluate, commentFor, hasAgreeCommand, isAgreement, findAgreement, isExempt, parseIds, parseRecord, buildEntry, isSigned, recordSignature, askBody, agreedBody } = new Function( + `${block}\nreturn { decideStatus, evaluate, commentFor, hasAgreeCommand, isAgreement, findAgreement, isExempt, parseIds, parseRecord, buildEntry, isSigned, recordSignature, askBody, agreedBody };`, )(); const T = "2026-11-08T10:00:00Z"; @@ -209,3 +209,147 @@ test("recordSignature gives up after the attempt limit and rethrows other errors await assert.rejects(recordSignature({ ...broken, entry: { github_id: 8 }, sleep: noSleep }), { status: 500 }); assert.equal(broken.state.writes, 1); }); + +// ---- the status decision: success only after the record holds the person ---- + +const WRITE_FAILED = "Could not record your agreement. Comment /orbynAgree again to try again."; +const ASK = "Comment /orbynAgree once to agree to the Orbyn contributor agreement."; + +test("decideStatus: exempt, already signed, unsigned", () => { + assert.deepEqual(decideStatus({ exempt: true }), { state: "success", description: "No agreement needed", comment: "none", throws: false }); + const signed = decideStatus({ signed: true }); + assert.equal(signed.state, "success"); + assert.equal(signed.throws, false); + assert.deepEqual(decideStatus({}), { state: "failure", description: ASK, comment: "ask", throws: false }); +}); + +test("decideStatus: a read error is an error status and fails the run", () => { + assert.deepEqual(decideStatus({ readError: new Error("x") }), { + state: "error", description: "Could not check the agreement record.", comment: "none", throws: true, + }); +}); + +test("decideStatus: a failed write is a failure with the exact line, and the run fails", () => { + const decision = decideStatus({ agreementFound: true, writeError: new Error("x") }); + assert.deepEqual(decision, { state: "failure", description: WRITE_FAILED, comment: "write-failed", throws: true }); + assert.ok(decision.description.length <= 140); +}); + +test("decideStatus: success is never returned unless exempt, signed, or the write succeeded", () => { + const flags = [true, false]; + for (const exempt of flags) for (const readError of [undefined, new Error("r")]) for (const signed of flags) + for (const agreementFound of flags) for (const writeOk of flags) for (const writeError of [undefined, new Error("w")]) { + const decision = decideStatus({ exempt, readError, signed, agreementFound, writeOk, writeError }); + if (decision.state !== "success") continue; + const allowed = exempt || (!readError && signed) || (!readError && agreementFound && writeOk && !writeError); + assert.ok(allowed, JSON.stringify({ exempt, readError: !!readError, signed, agreementFound, writeOk, writeError: !!writeError })); + } + // A write that was attempted and rejected, with or without writeOk, is never success. + assert.notEqual(decideStatus({ agreementFound: true, writeOk: true, writeError: new Error("w") }).state, "success"); + assert.notEqual(decideStatus({ agreementFound: true }).state, "success"); +}); + +// ---- evaluate: drives read, find and record the way the workflow does ---- + +const status = (status) => Object.assign(new Error(`HTTP ${status}`), { status }); +const agreementComment = comment("/orbynAgree", { id: 1 }); + +function harness({ initial = [], failures = [], sha = "s0", entry = { github_id: 1 } } = {}) { + const store = fakeStore(initial, failures, sha); + return { + store, + run: (overrides = {}) => evaluate({ + exempt: false, + userId: 1, + read: store.read, + findAgreementNow: () => agreementComment, + record: () => recordSignature({ ...store, entry, attempts: 3, sleep: noSleep }), + ...overrides, + }), + }; +} + +test("evaluate: exempt never touches the record", async () => { + const result = await evaluate({ exempt: true, userId: 1, read: () => assert.fail("read"), findAgreementNow: () => assert.fail("find"), record: () => assert.fail("record") }); + assert.equal(result.state, "success"); + assert.equal(result.description, "No agreement needed"); +}); + +test("evaluate: already in the record is success without writing", async () => { + const { store, run } = harness({ initial: [{ github_id: 1 }] }); + const result = await run(); + assert.equal(result.state, "success"); + assert.equal(store.state.writes, 0); +}); + +test("evaluate: no agreement comment stays failing with the /orbynAgree line", async () => { + const { run } = harness(); + const result = await run({ findAgreementNow: () => undefined }); + assert.deepEqual([result.state, result.description, result.comment], ["failure", ASK, "ask"]); +}); + +test("evaluate: success only after the write has resolved", async () => { + const { store, run } = harness(); + let writtenWhenDecided; + const result = await run({ + record: async (agreement) => { + await recordSignature({ ...store, entry: { github_id: 1 }, sleep: noSleep }); + writtenWhenDecided = store.state.list.some((e) => e.github_id === 1); + }, + }); + assert.equal(result.state, "success"); + assert.equal(result.comment, "agreed"); + assert.equal(writtenWhenDecided, true); + assert.equal(result.agreement, agreementComment); +}); + +test("evaluate: a write that rejects is never success (409 exhausted, 500, 403 token)", async () => { + for (const failures of [[status(409), status(409), status(409)], [status(500)], [status(403)]]) { + const { store, run } = harness({ failures }); + const result = await run(); + assert.notEqual(result.state, "success", failures[0].message); + assert.equal(result.state, "failure"); + assert.equal(result.description, WRITE_FAILED); + assert.equal(result.comment, "write-failed"); + assert.equal(result.throws, true); + assert.equal(result.error.status, failures[0].status); + assert.equal(store.state.list.some((e) => e.github_id === 1), false); + } +}); + +test("evaluate: a failure inside record before the write (such as reading CLA.md) is a write failure", async () => { + const { run } = harness(); + const result = await run({ record: async () => { throw status(404); } }); + assert.deepEqual([result.state, result.description, result.throws], ["failure", WRITE_FAILED, true]); +}); + +test("evaluate: a read error is an error status, never success", async () => { + const { run } = harness(); + const result = await run({ read: async () => { throw status(500); } }); + assert.deepEqual([result.state, result.description, result.throws], ["error", "Could not check the agreement record.", true]); + assert.equal(result.error.status, 500); +}); + +test("evaluate: the retry after a failed write records normally", async () => { + const { store, run } = harness({ failures: [status(500)] }); + const first = await run(); + assert.equal(first.state, "failure"); + const second = await run(); + assert.equal(second.state, "success"); + assert.equal(second.comment, "agreed"); + assert.equal(store.state.list.filter((e) => e.github_id === 1).length, 1); + // A third run (the next push) finds the person in the record and writes nothing more. + const writes = store.state.writes; + assert.equal((await run()).state, "success"); + assert.equal(store.state.writes, writes); +}); + +test("commentFor: the write-failure comment carries the marker and the retry line", () => { + const body = commentFor("write-failed", { login: "newcomer" }); + assert.match(body, //); + assert.match(body, /Comment `\/orbynAgree` again to try again/); + assert.equal(commentFor("none", { login: "x" }), null); + assert.equal(commentFor("agreed-if-present", { login: "x", hasOurs: false }), null); + assert.match(commentFor("agreed-if-present", { login: "x", hasOurs: true }), /all Orbyn repositories/); + assert.match(commentFor("ask", { login: "x" }), /`\/orbynAgree`/); +});