diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index bf07561..253488d 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -9,7 +9,9 @@ New to the project? See the "Your first PR" section in CONTRIBUTING.md. ## Checklist -- [ ] Commits are **DCO signed off** (`git commit -s`). No CLA is required. +- [ ] Commits are **DCO signed off** (`git commit -s`). No CLA is required. This is + enforced by the `dco` check; to sign commits you already made, see + [CONTRIBUTING.md](../CONTRIBUTING.md#sign-off-is-required). - [ ] If this changes a recorded decision in `docs/adr/`, an ADR is **added or supersedes** the affected record — with the argument, not just a status flip. - [ ] If the schema changed, I edited the Zod source diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 19bf5cb..51e5711 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -139,3 +139,37 @@ jobs: exit 0 fi bun run adr check "${changed[@]}" + + dco: + # ADR-0006 chose a DCO over a CLA. CONTRIBUTING.md and the PR template have + # required sign-off since the repository went public, and nothing checked it + # — an unsigned commit would have merged. This is that check (issue #130). + # + # A repository script rather than the DCO GitHub App, so the gate stays + # inside the surface ADR-0007 keeps mechanical and self-contained. It imports + # only Node builtins and therefore runs with no `bun install`: a broken + # dependency graph cannot take the sign-off gate down with it. + if: github.event_name == 'pull_request' + runs-on: ubuntu-latest + permissions: + contents: read + steps: + - name: Check out repository + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + fetch-depth: 0 + persist-credentials: false + - name: Set up Bun + uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2.2.0 + with: + bun-version: 1.3.14 + - name: Verify every commit carries a DCO sign-off + env: + # Explicit SHAs, not `origin/$BASE_REF..HEAD`. A ref resolves to + # whatever it points at now, so a branch that moved mid-run silently + # changes which commits were checked — the stale-read failure + # ADR-0016 records under "report what was examined". The default + # checkout is the synthetic merge ref, and this range excludes it. + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + run: bun run check:dco "$BASE_SHA..$HEAD_SHA" diff --git a/CHANGELOG.md b/CHANGELOG.md index 013f466..01e8c4c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,42 @@ Until `1.0.0`, minor releases may include breaking changes ### Added +- **DCO sign-off is enforced, not just documented** ([ADR-0006](docs/adr/0006-license-apache-2-and-single-monorepo.md) + action item 2, #130). A `dco` job checks every commit a pull request adds and is a + required status check on `main`. Sign-off was practiced by every contributor and + required by both `CONTRIBUTING.md` and the PR template, but no ruleset check + enforced it — the control was honor-system, and an unsigned commit would have + merged. + + **A repository script (`scripts/check-dco.ts`), not the [DCO app](https://github.com/apps/dco),** + so the gate stays inside the surface [ADR-0007](docs/adr/0007-adapter-isolation-and-public-surface-build.md) + keeps mechanical and self-contained rather than adding a third-party app to the IP + boundary. It imports only Node builtins and runs with no `bun install`, so a broken + dependency graph cannot take the sign-off gate down with it. Accept/reject semantics + track the app's, because that is the contract contributors already know, with two + deliberate differences: a sign-off's **name and address must come from one identity** + (the app takes the name from either the author or the committer and the address from + either, so a web-UI commit signed `Jane Doe ` passes there), and + a **bot still has to sign** — app accounts are exempt from the *address* half only, + because Dependabot signs from `support@github.com` and cannot equal its own author + address by construction, but the trailer must still name the bot. Every exemption is + named in the job output, so a commit is never skipped silently. + + **The squash-merge body setting moved from `BLANK` to `COMMIT_MESSAGES`.** A + pull-request check certifies the *contributor*, which is what the DCO is for, but a + blank squash body discards every trailer at merge: `main`'s own head (`f74c089`) + carried no sign-off while every commit proposed to it carried one. ADR-0006 traded + away commercial leverage for provenance, and provenance that is verified and then + thrown away at merge is not provenance. + + Observed rejecting a real unsigned commit in a real repository before it counted as + coverage ([ADR-0016](docs/adr/0016-require-every-check-to-be-observed-failing-before-it-counts-as-coverage.md)). + The negative cases are permanent in `scripts/check-dco.test.ts`, including the one + that matters most for this class of check: **an empty commit range is an error, not + a pass.** An unfetched or misspelled base ref makes `git log` return nothing and + drives every count in the report to zero, which renders identically to a clean run — + the exact fail-quiet shape ADR-0016 exists to prevent. + - **Badges — corpus size and ARB queue depth — as recipes over output adrkit already produces.** A new [badges guide](https://adrkit.dev/badges/) documents two snippets, both rendering a number through shields.io from JSON your own repository @@ -72,8 +108,10 @@ Until `1.0.0`, minor releases may include breaking changes trail says when the decision was taken and by whom rather than leaving `status` to carry it alone. Stale action-item checkboxes were corrected to match verified reality; items that are genuinely open were left unchecked, including ADR-0006's - DCO bot, which is documented in CONTRIBUTING.md but which no ruleset check - enforces, ADR-0008's re-import pull request, which is unbuilt because non-MADR + DCO bot, which CONTRIBUTING.md required but which no ruleset check enforced at the + time of that ratification — closed later in this same release by the DCO gate + described above — ADR-0008's re-import pull request, which is unbuilt because + non-MADR re-import is unbuilt, and ADR-0009's catalog port item — the port type exists but no adapter implementation ships, since `catalog-backstage` is placement and dependency boundary only. Ratification rests on the resolution semantics, which diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0470245..1e9cbac 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -53,6 +53,26 @@ why. git commit -s -m "your message" ``` +**This is enforced.** The `dco` job checks every commit your pull request adds and +is a required status check, so an unsigned commit cannot merge. If you forgot, sign +the whole branch at once and force-push: + +``` +git rebase --signoff origin/main +git push --force-with-lease +``` + +The trailer must name you: `Signed-off-by: Your Name `, matching the +commit's author or committer exactly. Merge commits are exempt — the commits they +merge carry the certification. Bot accounts are exempt from the address half only, +because they sign from a service address; their trailer must still name them. + +**Editing in the browser?** A commit made through the GitHub web editor carries no +sign-off, and you cannot add one from the browser. Clone the branch, run the rebase +above, and force-push — or make the change locally with `git commit -s` to begin +with. This catches docs-only contributions in particular, so it is worth knowing +before you start rather than after the check goes red. + ## Two hard rules These are enforced in CI. A PR that violates either will fail, and the fix is to diff --git a/MANIFEST.md b/MANIFEST.md index 87e57d2..5588460 100644 --- a/MANIFEST.md +++ b/MANIFEST.md @@ -31,7 +31,7 @@ adrkit/ │ └── adapters/ │ └── spec-kit/ @adrkit/spec-kit — independently versioned │ (ADR-0007); ships no dist, no dependencies -├── scripts/ release pack/publish, dep boundary + audit gates +├── scripts/ release pack/publish, dep boundary + audit + DCO gates ├── site/ Astro Starlight docs site; hosts the schema at its $id ├── specs/ 001–009, one spec-kit feature per phase/spike └── docs/ diff --git a/docs/adr/0006-license-apache-2-and-single-monorepo.md b/docs/adr/0006-license-apache-2-and-single-monorepo.md index 0c7a8af..c33ca30 100644 --- a/docs/adr/0006-license-apache-2-and-single-monorepo.md +++ b/docs/adr/0006-license-apache-2-and-single-monorepo.md @@ -15,6 +15,17 @@ affects: pattern: "LICENSE" - type: path pattern: "CONTRIBUTING.md" +assertions: + - id: dco-signoff-required + description: >- + Every commit a pull request adds must carry a Signed-off-by trailer naming + its author or its committer. Merge commits are exempt; app accounts are + exempt from the address half only, because they sign from a service + address, and their trailer must still name them. + engine: custom + expression: dco-signoff-required + input: source + severity: error provenance: authoredBy: agent-drafted ratifiedBy: "@mbeacom" @@ -147,7 +158,23 @@ sense if standardization is genuinely the goal — which ADR-0002 asserts it is. ## Action items 1. [x] LICENSE, NOTICE, per-package `license` fields -2. [ ] DCO bot enabled on the repository +2. [x] DCO sign-off enforced on the repository. **Done 2026-08-12** (#130) as the + `dco` job in `.github/workflows/ci.yml`, a required check on the `main` + ruleset, backed by `scripts/check-dco.ts`. A repository script rather than + the [DCO app](https://github.com/apps/dco), so the gate stays inside the + surface [ADR-0007](0007-adapter-isolation-and-public-surface-build.md) + keeps mechanical and self-contained; it imports only Node builtins, so a + broken dependency graph cannot take the sign-off gate down with it. + Observed rejecting a real unsigned commit before it counted as coverage + ([ADR-0016](0016-require-every-check-to-be-observed-failing-before-it-counts-as-coverage.md)), + with the negative cases kept in `scripts/check-dco.test.ts`. + + The repository's squash-merge body setting moved from `BLANK` to + `COMMIT_MESSAGES` in the same change. A pull-request check certifies the + *contributor*, which is what the DCO is for, but a blank squash body + discards every trailer at merge — so `main` carried unsigned commits + (`f74c089`) while every commit proposed to it was signed. Provenance that + is verified and then thrown away is not provenance. 3. [x] `schema/LICENSE` (CC0) with the carve-out stated plainly in the README 4. [x] SECURITY.md and CODE_OF_CONDUCT.md before the repository goes public 5. [ ] Resolve external participation obligations before first public push diff --git a/package.json b/package.json index 4a56cfb..0c54800 100644 --- a/package.json +++ b/package.json @@ -33,6 +33,7 @@ "check:changelog": "bun run scripts/check-changelog.ts", "check:freeze-hashes": "bun run scripts/check-freeze-hashes.ts", "check:doc-pins": "bun run scripts/check-doc-cli-versions.ts", + "check:dco": "bun run scripts/check-dco.ts", "audit:gate": "bun run scripts/audit-gate.ts", "adr": "bun packages/cli/src/index.ts", "site:queue": "bun packages/cli/src/index.ts queue --format json > site/public/queue.json", diff --git a/scripts/check-dco.test.ts b/scripts/check-dco.test.ts new file mode 100644 index 0000000..311d4d6 --- /dev/null +++ b/scripts/check-dco.test.ts @@ -0,0 +1,492 @@ +import { afterEach, describe, expect, test } from 'bun:test'; +import { execFileSync } from 'node:child_process'; +import { resolve } from 'node:path'; +import { + findDcoViolations, + isBotIdentity, + parseGitLog, + parseSignoffs, + type Commit, + type Identity, +} from './check-dco.ts'; +import { cleanupTestDir, resetTestDir } from '../packages/core/test/helpers.ts'; + +const DIR_NAME = 'check-dco'; +const SCRIPT = resolve(import.meta.dir, 'check-dco.ts'); + +const HUMAN: Identity = { name: 'Jane Doe', email: 'jane@example.com' }; +/** The identity GitHub records as committer for a squash or web-UI commit. */ +const WEB_FLOW: Identity = { name: 'GitHub', email: 'noreply@github.com' }; +/** Observed on `dependabot[bot]`'s commits in this repository (PR #99, 2df8d2bc). */ +const DEPENDABOT: Identity = { + name: 'dependabot[bot]', + email: '49699333+dependabot[bot]@users.noreply.github.com', +}; + +function commit(overrides: Partial = {}): Commit { + return { + sha: 'a'.repeat(40), + parents: ['b'.repeat(40)], + author: HUMAN, + committer: HUMAN, + message: 'feat: a thing\n\nSigned-off-by: Jane Doe \n', + ...overrides, + }; +} + +afterEach(async () => { + await cleanupTestDir(DIR_NAME); +}); + +describe('parseSignoffs', () => { + test('reads the name and address from a well-formed trailer', () => { + expect(parseSignoffs('subject\n\nSigned-off-by: Jane Doe \n')).toEqual([ + { name: 'Jane Doe', email: 'jane@example.com' }, + ]); + }); + + test('reads every trailer, in the order written', () => { + const message = + 'subject\n\nSigned-off-by: Jane Doe \nSigned-off-by: Sam Roe \n'; + expect(parseSignoffs(message)).toEqual([ + { name: 'Jane Doe', email: 'jane@example.com' }, + { name: 'Sam Roe', email: 'sam@example.com' }, + ]); + }); + + test('reads a trailer that is not in the final paragraph', () => { + const message = + 'subject\n\nSigned-off-by: Jane Doe \nCo-authored-by: Sam Roe \n'; + expect(parseSignoffs(message)).toEqual([{ name: 'Jane Doe', email: 'jane@example.com' }]); + }); + + test('tolerates \\r\\n line endings', () => { + expect(parseSignoffs('subject\r\n\r\nSigned-off-by: Jane Doe \r\n')).toEqual([ + { name: 'Jane Doe', email: 'jane@example.com' }, + ]); + }); + + // Negative case: a trailer is line-anchored, so a mid-line mention is prose. + // Without the anchor this parses as a sign-off and a commit that merely + // *discusses* the format certifies itself. + test('does not read a mention that is not at the start of a line', () => { + expect(parseSignoffs('subject\n\nadds Signed-off-by: Jane Doe parsing\n')).toEqual( + [], + ); + }); + + // Negative case for the `[ \t\r]*$` terminator. Under the `m` flag a greedy + // `\s*$` runs past the trailer's own line, so a following line is swallowed + // into the match and the addresses stop lining up. + test('stops the trailer at its own line when more text follows', () => { + const message = 'subject\n\nSigned-off-by: Jane Doe \n\ntrailing prose\n'; + expect(parseSignoffs(message)).toEqual([{ name: 'Jane Doe', email: 'jane@example.com' }]); + }); + + // Negative case for `[^<>]*` over the reference implementation's greedy `(.*)`, + // which captures `jane@example.com> { + expect( + parseSignoffs('subject\n\nSigned-off-by: Jane Doe \n'), + ).toEqual([]); + }); + + // Negative case: `[^<>]` alone also matches a newline, so a line reading + // exactly `Signed-off-by:` let the lazy name group run down the message to the + // next `<` and swallow the real trailer beneath it, yielding one match named + // `Signed-off-by: Jane Doe`. That destroyed the only valid candidate and failed + // a correctly signed commit — the merge-blocking direction, and a direct + // falsification of this parser's own "can only ever add a candidate" claim. + test('does not let a bare Signed-off-by label swallow the trailer below it', () => { + const message = 'feat: thing\n\nSigned-off-by:\nSigned-off-by: Jane Doe \n'; + expect(parseSignoffs(message)).toEqual([{ name: 'Jane Doe', email: 'jane@example.com' }]); + }); + + test('does not let an address-less trailer reach across intervening lines', () => { + const message = + 'feat: thing\n\nSigned-off-by: Jane Doe\nsome prose\n\nSigned-off-by: Jane Doe \n'; + expect(parseSignoffs(message)).toEqual([{ name: 'Jane Doe', email: 'jane@example.com' }]); + }); + + // The same line confinement caps backtracking at one line. Before it, three + // quantifiers that can all match the same characters re-scanned the rest of the + // message on every retry, so a long bracket-free run was superlinear. A generous + // bound: the point is that it completes at all, not the exact millisecond. + test('parses a pathologically long bracket-free line without blowing up', () => { + const message = + 'x\n\nSigned-off-by: ' + 'a'.repeat(200_000) + '\nSigned-off-by: Jane Doe \n'; + const started = performance.now(); + expect(parseSignoffs(message)).toEqual([{ name: 'Jane Doe', email: 'jane@example.com' }]); + expect(performance.now() - started).toBeLessThan(2000); + }); + + test('finds nothing in a message with no trailer at all', () => { + expect(parseSignoffs('feat: a thing\n\nA body with no trailer.\n')).toEqual([]); + }); +}); + +describe('isBotIdentity', () => { + test('recognises the app account observed on this repository', () => { + expect(isBotIdentity(DEPENDABOT)).toBe(true); + }); + + test('does not recognise a human', () => { + expect(isBotIdentity(HUMAN)).toBe(false); + }); + + // Negative case: the `[bot]` suffix alone is a string anyone can type into + // `user.name`. Requiring the numeric-id noreply address too is what stops a + // human from renaming themselves into the identity exemption. + test('does not recognise a name suffix without the app noreply address', () => { + expect(isBotIdentity({ name: 'totally-a[bot]', email: 'jane@example.com' })).toBe(false); + }); + + test('does not recognise an app noreply address without the name suffix', () => { + expect( + isBotIdentity({ name: 'Jane Doe', email: '49699333+dependabot[bot]@users.noreply.github.com' }), + ).toBe(false); + }); +}); + +describe('findDcoViolations', () => { + test('accepts a commit signed by its author', () => { + const report = findDcoViolations([commit()]); + expect(report).toEqual({ examined: 1, signed: 1, exemptions: [], violations: [] }); + }); + + test('accepts a sign-off matching the committer when the author differs', () => { + const report = findDcoViolations([ + commit({ + author: { name: 'Sam Roe', email: 'sam@example.com' }, + committer: HUMAN, + }), + ]); + expect(report.violations).toEqual([]); + expect(report.signed).toBe(1); + }); + + test('compares case-insensitively', () => { + const report = findDcoViolations([ + commit({ message: 'feat: a thing\n\nSigned-off-by: JANE DOE \n' }), + ]); + expect(report.signed).toBe(1); + }); + + test('accepts when one of several trailers matches', () => { + const report = findDcoViolations([ + commit({ + message: + 'feat: a thing\n\nSigned-off-by: Sam Roe \nSigned-off-by: Jane Doe \n', + }), + ]); + expect(report.signed).toBe(1); + }); + + // Negative case: the reason this check exists. Observed rejecting the shape of + // `f74c089`, the unsigned squash commit that was on `main` when it was written. + test('rejects a commit with no sign-off', () => { + const report = findDcoViolations([ + commit({ sha: 'f74c089d4807d99d2d3f7d1cbb49dac40348d8d2', message: 'docs(adr): ratify\n\nA body.\n' }), + ]); + expect(report.signed).toBe(0); + expect(report.violations).toEqual([ + { + sha: 'f74c089d4807d99d2d3f7d1cbb49dac40348d8d2', + subject: 'docs(adr): ratify', + author: 'Jane Doe ', + detail: 'the sign-off is missing', + }, + ]); + }); + + // Negative case: a sign-off naming somebody who did not touch the commit + // certifies nothing, which is the failure a presence-only check cannot see. + test('rejects a sign-off by an identity that is neither author nor committer', () => { + const report = findDcoViolations([ + commit({ message: 'feat: a thing\n\nSigned-off-by: Sam Roe \n' }), + ]); + expect(report.violations).toHaveLength(1); + expect(report.violations[0]?.detail).toBe( + 'expected a sign-off by "Jane Doe ", but got "Sam Roe "', + ); + }); + // Negative case for pairing the halves of an identity. The DCO app takes the + // name from either side and the address from either side, so this exact commit + // passes there: `Jane Doe` is the author's name and `noreply@github.com` the + // committer's address, yet nobody ever signed as that pair. + test('rejects a name and address drawn from two different identities', () => { + const report = findDcoViolations([ + commit({ + committer: WEB_FLOW, + message: 'feat: a thing\n\nSigned-off-by: Jane Doe \n', + }), + ]); + expect(report.signed).toBe(0); + expect(report.violations).toHaveLength(1); + }); + + test('exempts a merge commit, and says so', () => { + const report = findDcoViolations([ + commit({ parents: ['b'.repeat(40), 'c'.repeat(40)], message: 'Merge pull request #22\n' }), + ]); + expect(report.violations).toEqual([]); + expect(report.exemptions).toHaveLength(1); + expect(report.exemptions[0]?.reason).toBe('merge'); + }); + + test('exempts an app account from the identity match, and says so', () => { + const report = findDcoViolations([ + commit({ + author: DEPENDABOT, + committer: WEB_FLOW, + message: 'build(deps): bump x\n\nSigned-off-by: dependabot[bot] \n', + }), + ]); + expect(report.violations).toEqual([]); + expect(report.exemptions).toHaveLength(1); + expect(report.exemptions[0]?.reason).toBe('bot'); + expect(report.exemptions[0]?.detail).toBe( + 'signed by the app account as dependabot[bot] ', + ); + }); + + // Negative case for the one place this is stricter than the DCO app, which + // skips app-authored commits outright. Exempting the address must not silently + // exempt presence too. + test('rejects an app account that did not sign at all', () => { + const report = findDcoViolations([ + commit({ author: DEPENDABOT, committer: WEB_FLOW, message: 'build(deps): bump x\n' }), + ]); + expect(report.exemptions).toEqual([]); + expect(report.violations).toHaveLength(1); + expect(report.violations[0]?.detail).toBe('the sign-off is missing'); + }); + + // Negative case: exempting only the *address* is the point. A presence-only + // exemption accepts an unrelated person's trailer on a bot's commit and then + // reports it as "signed by the app account" — a report asserting something it + // never checked, which is the ADR-0016 failure the exemption exists inside. + test('rejects an app account whose only sign-off names somebody else', () => { + const report = findDcoViolations([ + commit({ + author: DEPENDABOT, + committer: WEB_FLOW, + message: 'build(deps): bump x\n\nSigned-off-by: Jane Doe \n', + }), + ]); + expect(report.exemptions).toEqual([]); + expect(report.violations).toHaveLength(1); + expect(report.violations[0]?.detail).toContain('dependabot[bot]'); + expect(report.violations[0]?.detail).toContain('but got "Jane Doe "'); + }); + + test('exempts an app account on the trailer that names it, among several', () => { + const report = findDcoViolations([ + commit({ + author: DEPENDABOT, + committer: WEB_FLOW, + message: + 'build(deps): bump x\n\nSigned-off-by: Jane Doe \nSigned-off-by: dependabot[bot] \n', + }), + ]); + expect(report.violations).toEqual([]); + expect(report.exemptions[0]?.detail).toBe( + 'signed by the app account as dependabot[bot] ', + ); + }); + + // The classifier accepts a sign-off matching the author *or* the committer, so + // a diagnostic naming only the author omits a valid way to fix the commit. + test('names both identities in the diagnostic when they differ', () => { + const report = findDcoViolations([ + commit({ + author: { name: 'Sam Roe', email: 'sam@example.com' }, + committer: WEB_FLOW, + message: 'feat: a thing\n\nSigned-off-by: Nobody Here \n', + }), + ]); + expect(report.violations[0]?.detail).toBe( + 'expected a sign-off by "Sam Roe " or "GitHub ", ' + + 'but got "Nobody Here "', + ); + }); + + test('names one identity when author and committer are the same', () => { + const report = findDcoViolations([ + commit({ message: 'feat: a thing\n\nSigned-off-by: Nobody Here \n' }), + ]); + expect(report.violations[0]?.detail).toBe( + 'expected a sign-off by "Jane Doe ", but got "Nobody Here "', + ); + }); + + test('reports every commit it examined, not only the failing ones', () => { + const report = findDcoViolations([ + commit({ sha: '1'.repeat(40) }), + commit({ sha: '2'.repeat(40), parents: ['x', 'y'] }), + commit({ sha: '3'.repeat(40), message: 'feat: unsigned\n' }), + ]); + expect(report.examined).toBe(3); + expect(report.signed).toBe(1); + expect(report.exemptions).toHaveLength(1); + expect(report.violations).toHaveLength(1); + }); + + test('examines nothing and reports nothing for an empty range', () => { + expect(findDcoViolations([])).toEqual({ + examined: 0, + signed: 0, + exemptions: [], + violations: [], + }); + }); +}); + +describe('parseGitLog', () => { + const record = (sha: string, parents: string, message: string) => + [sha, parents, 'Jane Doe', 'jane@example.com', 'Jane Doe', 'jane@example.com', message].join('\0'); + + test('parses a single NUL-terminated record', () => { + const commits = parseGitLog(`${record('abc', 'def', 'subject\n')}\0`); + expect(commits).toEqual([ + { + sha: 'abc', + parents: ['def'], + author: HUMAN, + committer: HUMAN, + message: 'subject\n', + }, + ]); + }); + + test('splits a merge commit into both parents', () => { + const commits = parseGitLog(`${record('abc', 'def 012', 'merge\n')}\0`); + expect(commits[0]?.parents).toEqual(['def', '012']); + }); + + test('gives a root commit no parents', () => { + expect(parseGitLog(`${record('abc', '', 'first\n')}\0`)[0]?.parents).toEqual([]); + }); + + test('keeps a message containing blank lines and newlines intact', () => { + const message = 'subject\n\nbody line one\n\nSigned-off-by: Jane Doe \n'; + expect(parseGitLog(`${record('abc', 'def', message)}\0`)[0]?.message).toBe(message); + }); + + test('returns nothing for empty output', () => { + expect(parseGitLog('')).toEqual([]); + }); + + // Negative case: dropping a partial record would check fewer commits than the + // range held and still report success — a count that cannot tell "looked and + // found nothing" from "could not look" (ADR-0016). + test('throws rather than dropping a partial trailing record', () => { + expect(() => parseGitLog(`${record('abc', 'def', 'subject\n')}\0xyz\0`)).toThrow( + /not a multiple of 7/, + ); + }); +}); + +/** + * Run the script end to end against a real repository. The unit tests above prove + * the kernel classifies correctly; these prove the wiring — `git log` framing, + * exit code, and the empty-range guard — which is where a check most easily + * becomes green by failing to look. + */ +describe('check-dco end to end', () => { + const git = (cwd: string, ...args: string[]) => + execFileSync('git', args, { cwd, encoding: 'utf8', env: { ...process.env, GIT_CONFIG_GLOBAL: '/dev/null' } }); + + async function repoWith(commits: readonly { message: string }[]): Promise { + const root = await resetTestDir(DIR_NAME); + git(root, 'init', '--quiet', '--initial-branch=main'); + git(root, 'config', 'user.name', HUMAN.name); + git(root, 'config', 'user.email', HUMAN.email); + git(root, 'commit', '--quiet', '--allow-empty', '-m', `base\n\nSigned-off-by: ${HUMAN.name} <${HUMAN.email}>`); + git(root, 'branch', 'base'); + for (const { message } of commits) { + git(root, 'commit', '--quiet', '--allow-empty', '-m', message); + } + return root; + } + + function run(cwd: string, range: string) { + const result = Bun.spawnSync(['bun', 'run', SCRIPT, range], { cwd }); + return { + code: result.exitCode, + stdout: result.stdout.toString(), + stderr: result.stderr.toString(), + }; + } + + test('passes on a signed commit and names what it examined', async () => { + const root = await repoWith([ + { message: `feat: signed\n\nSigned-off-by: ${HUMAN.name} <${HUMAN.email}>` }, + ]); + const result = run(root, 'base..HEAD'); + expect(result.stdout).toContain('examined 1 commit(s) in base..HEAD'); + expect(result.stdout).toContain('ok — 1 signed, 0 exempt, 0 unsigned'); + expect(result.code).toBe(0); + }); + + // The permanent negative case required by ADR-0016: a real unsigned commit in a + // real repository, observed being rejected. Watching the kernel reject a struct + // literal proves the classifier; only this proves the script fails the build. + test('fails on a real unsigned commit', async () => { + const root = await repoWith([{ message: 'feat: unsigned' }]); + const result = run(root, 'base..HEAD'); + expect(result.code).toBe(1); + expect(result.stderr).toContain('1 of 1 commit(s) lack a valid DCO sign-off'); + expect(result.stderr).toContain('feat: unsigned'); + expect(result.stderr).toContain('the sign-off is missing'); + expect(result.stderr).toContain('git rebase --signoff base'); + }); + + // Negative case for the merge-blocking direction, end to end: this commit is + // correctly signed, and the parser used to report it unsigned and fail the + // build. A false positive on a required check blocks every merge, so it is the + // more expensive of the two ways this check can be wrong. + test('passes a signed commit whose message also holds a bare Signed-off-by label', async () => { + const root = await repoWith([ + { message: `feat: thing\n\nSigned-off-by:\nSigned-off-by: ${HUMAN.name} <${HUMAN.email}>` }, + ]); + const result = run(root, 'base..HEAD'); + expect(result.stdout).toContain('ok — 1 signed, 0 exempt, 0 unsigned'); + expect(result.code).toBe(0); + }); + + test('fails on the unsigned commit while accepting its signed neighbour', async () => { + const root = await repoWith([ + { message: `feat: signed\n\nSigned-off-by: ${HUMAN.name} <${HUMAN.email}>` }, + { message: 'feat: unsigned' }, + ]); + const result = run(root, 'base..HEAD'); + expect(result.code).toBe(1); + expect(result.stderr).toContain('1 of 2 commit(s)'); + expect(result.stderr).toContain('feat: unsigned'); + expect(result.stderr).not.toContain('feat: signed'); + }); + + // Negative case for the fail-quiet shape itself. A misspelled or unfetched base + // ref makes `git log` return nothing, and every count in the report goes to + // zero — indistinguishable from a clean pass unless the empty range is an error. + test('refuses to pass an empty range instead of reporting zero commits', async () => { + const root = await repoWith([ + { message: `feat: signed\n\nSigned-off-by: ${HUMAN.name} <${HUMAN.email}>` }, + ]); + const result = run(root, 'HEAD..HEAD'); + expect(result.code).toBe(1); + expect(result.stderr).toContain('no commits in range HEAD..HEAD'); + expect(result.stderr).not.toContain('ok —'); + }); + + test('fails rather than passing when the base ref does not exist', async () => { + const root = await repoWith([ + { message: `feat: signed\n\nSigned-off-by: ${HUMAN.name} <${HUMAN.email}>` }, + ]); + const result = run(root, 'origin/nonexistent..HEAD'); + expect(result.code).toBe(1); + expect(result.stdout).not.toContain('ok —'); + }); +}); diff --git a/scripts/check-dco.ts b/scripts/check-dco.ts new file mode 100644 index 0000000..6cee963 --- /dev/null +++ b/scripts/check-dco.ts @@ -0,0 +1,399 @@ +/** + * Fail when a commit in a pull request carries no valid DCO sign-off. + * + * [ADR-0006](../docs/adr/0006-license-apache-2-and-single-monorepo.md) chose a + * DCO over a CLA because it "forecloses a rug-pull" while keeping contribution + * friction low. That trade only pays if sign-off is actually present, and until + * this check existed it was documented in `CONTRIBUTING.md` and enforced by + * nothing — an unsigned commit would have merged. + * + * Implemented as a repository script rather than by installing the + * [DCO app](https://github.com/apps/dco), so the check stays inside the surface + * ADR-0007 keeps mechanical and self-contained. The accept/reject semantics + * deliberately track that app's, because they are the contract contributors + * already know, with two differences noted at {@link findDcoViolations}. + * + * bun run scripts/check-dco.ts [range] + * + * `range` is any two-dot git revision range and defaults to + * `origin/main..HEAD`. CI passes the pull request's own range explicitly. + * + * **Known limitation (#137).** This runs from the pull request's own merge + * checkout, so a change that edits this file — or the workflow step invoking it — + * can produce a green `dco` status over unsigned commits. That is a property of + * every gate in this repository, not of this one: `pull_request` workflows + * execute the *pull request's* `ci.yml`, measured directly on #98. Moving this + * script to a trusted base revision would not close it, because the step calling + * it is equally under the pull request's control, and a control that looks like a + * control and is not one is worse than a documented gap (ADR-0016). Tracked + * repository-wide in #137 rather than papered over here. + */ + +import { execFileSync } from 'node:child_process'; + +export interface Identity { + name: string; + email: string; +} + +export interface Commit { + sha: string; + /** Empty for a root commit; more than one marks a merge. */ + parents: readonly string[]; + author: Identity; + committer: Identity; + /** Raw message, subject and body, exactly as `git log %B` renders it. */ + message: string; +} + +export interface Signoff { + name: string; + email: string; +} + +/** Why a commit was not required to match an identity. Always reported. */ +export type ExemptionReason = 'merge' | 'bot'; + +export interface Exemption { + sha: string; + subject: string; + author: string; + reason: ExemptionReason; + detail: string; +} + +export interface Violation { + sha: string; + subject: string; + author: string; + detail: string; +} + +export interface DcoReport { + /** Every commit the range produced, exempt or not. */ + examined: number; + /** Commits that carried a sign-off matching a required identity. */ + signed: number; + exemptions: Exemption[]; + violations: Violation[]; +} + +/** + * Source for the sign-off matcher. A fresh `RegExp` is built per scan rather + * than sharing one module-level instance: `matchAll` seeds its matcher from the + * source regex's `lastIndex`, so a shared global regex any caller had poked with + * `.exec()` would start mid-message and skip trailers. + * + * Anchored to the start of a physical line, because a sign-off is a trailer and + * a mid-line mention is prose. + * + * Every class excludes `\r` and `\n`, so no match can span a line boundary. This + * is load-bearing rather than tidy. `[^<>]` alone also matches a newline, so a + * line reading exactly `Signed-off-by:` — no address on it — let the lazy name + * group run *down the message* to the next `<`, swallowing the real trailer + * beneath it into one match whose name is `Signed-off-by: Jane Doe`. That + * removed the only valid candidate and failed a correctly signed commit, which + * is the merge-blocking direction. Confining to one line also caps the + * quantifiers' backtracking at a single line's length. + * + * Both halves further exclude `<` and `>`, which git's own ident parser forbids. + * The reference implementation captures the name greedily as `(.*)`, so on + * `Signed-off-by: Jane Doe ` it backtracks + * the first pair into the *name* and reads the address as `spoof@example.com`. + * Excluding the brackets from the name leaves the line with no parse at all, + * which is the honest reading of a trailer naming two addresses. + */ +const SIGNOFF_SOURCE = String.raw`^Signed-off-by:[ \t]*([^<>\r\n]+?)[ \t]*<([^<>\r\n]*)>[ \t\r]*$`; + +/** A fresh matcher. Never share one — see {@link SIGNOFF_SOURCE}. */ +export function signoffPattern(): RegExp { + return new RegExp(SIGNOFF_SOURCE, 'gim'); +} + +/** + * Pure: every `Signed-off-by` trailer in `message`, in the order written. + * + * Deliberately accepts a trailer anywhere in the message rather than only in the + * final paragraph. A stricter reading would reject a sign-off separated from the + * end by a `Co-authored-by` block or a revert footer, and a false rejection here + * blocks a merge — a far worse failure than tolerating a sign-off quoted in + * prose, which can only ever add a candidate and never remove a valid one. + */ +export function parseSignoffs(message: string): Signoff[] { + const found: Signoff[] = []; + for (const match of message.matchAll(signoffPattern())) { + found.push({ name: (match[1] as string).trim(), email: (match[2] as string).trim() }); + } + return found; +} + +/** Case-insensitive, whitespace-tolerant equality on the name half of an identity. */ +function sameName(signoff: Signoff, identity: Identity): boolean { + return signoff.name.trim().toLowerCase() === identity.name.trim().toLowerCase(); +} + +/** Case-insensitive, whitespace-tolerant equality on both halves of an identity. */ +function matchesIdentity(signoff: Signoff, identity: Identity): boolean { + return ( + sameName(signoff, identity) && + signoff.email.trim().toLowerCase() === identity.email.trim().toLowerCase() + ); +} + +/** A GitHub App account: `[bot]` with the noreply address GitHub issues it. */ +const BOT_NAME = /\[bot\]$/; +const BOT_EMAIL = /^\d+\+[^@\s]*\[bot\]@users\.noreply\.github\.com$/i; + +/** + * Pure: whether `identity` is a GitHub App account. + * + * Both halves are required. The name suffix alone is a string anyone can type + * into `user.name`; pairing it with the numeric-id noreply address GitHub issues + * to app accounts is what makes the claim cost something. Neither is verified by + * git, which is why every commit this exempts is named in the output rather than + * skipped quietly — a human who exempts themselves appears in the CI log. + */ +export function isBotIdentity(identity: Identity): boolean { + return BOT_NAME.test(identity.name.trim()) && BOT_EMAIL.test(identity.email.trim()); +} + +function subjectOf(message: string): string { + return (message.split('\n', 1)[0] ?? '').trim(); +} + +function render(identity: Identity): string { + return `${identity.name} <${identity.email}>`; +} + +/** + * Pure: classify every commit in `commits` as signed, exempt, or in violation. + * + * A commit needs a `Signed-off-by` trailer whose name **and address together** + * equal the author's or the committer's. Two rules differ from the DCO app: + * + * - **The pair must come from one identity.** The app accepts a name from the + * author and an address from the committer, so a web-UI commit signed + * `Jane Doe ` passes there. Pairing can only reject a + * sign-off that names nobody who touched the commit, so it rejects nothing a + * well-formed trailer produces. + * - **A bot still has to sign.** The app skips app-authored commits outright. + * They are exempted here from the *address* half of the match only, because a + * bot signs from a service address — Dependabot's author is + * `dependabot[bot] <…+dependabot[bot]@users.noreply.github.com>` while it signs + * `dependabot[bot] `, so the two cannot be equal by + * construction. The trailer must still *name* the bot, or the exemption would + * accept an unrelated person's signature on a bot's commit and report it as + * the app account's. + * + * Merge commits are exempt: the commits they merge carry the certification, and + * the person who ran `git merge` authored none of it. That reasoning covers the + * ordinary merge and not an evil merge, whose recorded tree can hold conflict + * resolution present in neither parent. The DCO app skips merges outright too, + * so this matches the contract contributors expect rather than closing that gap. + */ +export function findDcoViolations(commits: readonly Commit[]): DcoReport { + const exemptions: Exemption[] = []; + const violations: Violation[] = []; + let signed = 0; + + for (const commit of commits) { + const sha = commit.sha; + const subject = subjectOf(commit.message); + const author = render(commit.author); + + if (commit.parents.length > 1) { + exemptions.push({ sha, subject, author, reason: 'merge', detail: 'merge commit' }); + continue; + } + + const signoffs = parseSignoffs(commit.message); + if (signoffs.length === 0) { + violations.push({ + sha, + subject, + author, + detail: 'the sign-off is missing', + }); + continue; + } + + // An app account signs from a service address — Dependabot authors as + // `dependabot[bot] <…+dependabot[bot]@users.noreply.github.com>` and signs as + // `dependabot[bot] ` — so the address cannot match by + // construction. The *name* still can, and requiring it is what stops the + // exemption from accepting an unrelated person's trailer on a bot's commit + // and then reporting it as "signed by the app account". + if (isBotIdentity(commit.author)) { + const byBot = signoffs.find((signoff) => sameName(signoff, commit.author)); + if (byBot) { + exemptions.push({ + sha, + subject, + author, + reason: 'bot', + detail: `signed by the app account as ${render(byBot)}`, + }); + continue; + } + // Nothing here names the bot. Fall through to the identity match so this + // fails with the precise mismatch below, rather than being waved through + // on a signature that certifies somebody else's work. + } + + const matched = signoffs.some( + (signoff) => matchesIdentity(signoff, commit.author) || matchesIdentity(signoff, commit.committer), + ); + + if (matched) { + signed += 1; + continue; + } + + // Name both identities when they differ. The classifier accepts either, so a + // message naming only the author omits a valid way to fix the commit. + const committer = render(commit.committer); + const expected = + author.toLowerCase() === committer.toLowerCase() ? `"${author}"` : `"${author}" or "${committer}"`; + const got = signoffs.map((signoff) => `"${render(signoff)}"`).join(', '); + violations.push({ + sha, + subject, + author, + detail: `expected a sign-off by ${expected}, but got ${got}`, + }); + } + + return { examined: commits.length, signed, exemptions, violations }; +} + +/** Field order in {@link GIT_FORMAT}. Changing one without the other misparses. */ +const GIT_FIELDS = 7; + +/** + * NUL between every field and, with `-z`, between every record. A commit object + * cannot contain a NUL byte, so the framing is unambiguous for any message — + * unlike a printable delimiter, which a commit body may legitimately contain. + */ +const GIT_FORMAT = '%H%x00%P%x00%an%x00%ae%x00%cn%x00%ce%x00%B'; + +/** + * Pure: parse the NUL-framed output of `git log -z --format=`{@link GIT_FORMAT}. + * + * A token count that is not a whole number of records throws rather than + * dropping the remainder, because a partially-parsed range silently checks + * fewer commits than the range contained. + */ +export function parseGitLog(stdout: string): Commit[] { + const tokens = stdout.split('\0'); + // `-z` terminates the final record too, leaving one empty trailing token. + if (tokens.length > 0 && tokens[tokens.length - 1] === '') tokens.pop(); + if (tokens.length === 0) return []; + + if (tokens.length % GIT_FIELDS !== 0) { + throw new Error( + `git log produced ${tokens.length} fields, which is not a multiple of ${GIT_FIELDS}; ` + + `refusing to check a partially-parsed range`, + ); + } + + const commits: Commit[] = []; + for (let i = 0; i < tokens.length; i += GIT_FIELDS) { + commits.push({ + sha: (tokens[i] as string).trim(), + parents: (tokens[i + 1] as string).trim().split(/\s+/).filter(Boolean), + author: { name: tokens[i + 2] as string, email: tokens[i + 3] as string }, + committer: { name: tokens[i + 4] as string, email: tokens[i + 5] as string }, + message: tokens[i + 6] as string, + }); + } + return commits; +} + +export function readCommits(range: string, cwd?: string): Commit[] { + let stdout: string; + try { + stdout = execFileSync('git', ['log', '-z', `--format=${GIT_FORMAT}`, range], { + cwd, + encoding: 'utf8', + maxBuffer: 64 * 1024 * 1024, + }); + } catch (error) { + // Say that the range could not be *read*, not that commits were unsigned. + // git's own text here is `fatal: bad revision`, which reads like a defect in + // the pull request rather than in the checkout. The usual cause is a base + // commit that was never fetched — for a conflicting pull request GitHub may + // have no current merge ref, so the object the range names is simply absent. + const detail = error instanceof Error ? error.message.trim() : String(error); + throw new Error( + `could not read the commit range ${range}, so no commit was examined.\n` + + `This is a checkout problem, not a sign-off problem — the base or head commit is\n` + + `probably missing from this clone (fetch it, or resolve the pull request's conflicts).\n` + + `git said: ${detail}`, + ); + } + return parseGitLog(stdout); +} + +export function formatReport(range: string, report: DcoReport): string { + const lines = [`check-dco: examined ${report.examined} commit(s) in ${range}`]; + for (const exemption of report.exemptions) { + lines.push(` exempt ${exemption.sha.slice(0, 8)} ${exemption.subject} — ${exemption.detail}`); + } + return lines.join('\n'); +} + +function main(argv: readonly string[]): void { + const range = argv[0] ?? process.env.DCO_RANGE ?? 'origin/main..HEAD'; + const base = range.split('..')[0] || 'origin/main'; + const commits = readCommits(range); + + // A pull request always contains at least one commit, so an empty range means + // the base ref was never fetched or the range was misspelled — not that every + // commit passed. Reporting "0 commits, ok" here is the exact fail-quiet shape + // ADR-0016 exists to prevent, and it would render identically to a clean run. + if (commits.length === 0) { + throw new Error( + `no commits in range ${range}; the base ref is probably unfetched or misspelled. ` + + `Refusing to report a pass over an empty range.`, + ); + } + + const report = findDcoViolations(commits); + console.log(formatReport(range, report)); + + if (report.violations.length > 0) { + const detail = report.violations + .map( + (violation) => + ` ${violation.sha.slice(0, 8)} ${violation.subject}\n` + + ` author: ${violation.author}\n` + + ` problem: ${violation.detail}`, + ) + .join('\n\n'); + throw new Error( + `${report.violations.length} of ${report.examined} commit(s) lack a valid DCO sign-off:\n\n${detail}\n\n` + + `Every commit needs "Signed-off-by: Your Name " naming its author or its\n` + + `committer. Sign-off is your statement that you have the right to submit this work\n` + + `under the project's license — see https://developercertificate.org/.\n\n` + + `Sign the whole branch and force-push:\n` + + ` git rebase --signoff ${base}\n` + + ` git push --force-with-lease\n\n` + + `Use --force-with-lease, not --force: it refuses rather than discarding a commit\n` + + `someone else pushed. If git cannot find ${base}, run "git fetch origin" first.\n` + + `See CONTRIBUTING.md.`, + ); + } + + console.log( + `check-dco: ok — ${report.signed} signed, ${report.exemptions.length} exempt, 0 unsigned`, + ); +} + +if (import.meta.main) { + try { + main(process.argv.slice(2)); + } catch (error) { + console.error(`check-dco: ${error instanceof Error ? error.message : String(error)}`); + process.exit(1); + } +}