From ab02e1d7d74133eed81a0e3cede72046a713c89d Mon Sep 17 00:00:00 2001 From: Mark Beacom Date: Wed, 12 Aug 2026 08:48:20 -0400 Subject: [PATCH 1/4] ci(dco): enforce DCO sign-off as a required check ADR-0006 chose a DCO over a CLA because it forecloses a rug-pull while keeping contribution friction low. Sign-off was practiced by every contributor and required by both CONTRIBUTING.md and the PR template, but no ruleset check enforced it. An unsigned commit would have merged. 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 needs no bun install, so a broken dependency graph cannot take the sign-off gate down with it. Accept/reject semantics track the app's, since 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 identity match only, because Dependabot signs from support@github.com and cannot equal its own author address by construction, but presence is still checked. Every exemption is named in the output, so no commit is skipped silently. The squash-merge body setting moves 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: main's own head f74c089 carries no sign-off while every commit proposed to it carried one. Provenance verified and then thrown away is not provenance. Per ADR-0016 the check was observed rejecting a real unsigned commit in a real repository before it counted as coverage, and the negative cases are permanent. The one that matters most for this class of check is that an empty commit range is an error rather than a pass: an unfetched or misspelled base ref makes git log return nothing and drives every count to zero, which renders identically to a clean run. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom --- .github/workflows/ci.yml | 34 ++ CHANGELOG.md | 36 ++ CONTRIBUTING.md | 14 + MANIFEST.md | 2 +- ...06-license-apache-2-and-single-monorepo.md | 18 +- package.json | 1 + scripts/check-dco.test.ts | 393 ++++++++++++++++++ scripts/check-dco.ts | 335 +++++++++++++++ 8 files changed, 831 insertions(+), 2 deletions(-) create mode 100644 scripts/check-dco.test.ts create mode 100644 scripts/check-dco.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 19bf5cb..2be9038 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 scripts/check-dco.ts "$BASE_SHA..$HEAD_SHA" diff --git a/CHANGELOG.md b/CHANGELOG.md index 013f466..28f9006 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 identity match only, + because Dependabot signs from `support@github.com` and cannot equal its own author + address, but presence is still checked. 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 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0470245..e27d95c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -53,6 +53,20 @@ 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 identity match +only, because they sign from a service address; they still have to sign. + ## 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..fba7f62 100644 --- a/docs/adr/0006-license-apache-2-and-single-monorepo.md +++ b/docs/adr/0006-license-apache-2-and-single-monorepo.md @@ -147,7 +147,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..a07efbb --- /dev/null +++ b/scripts/check-dco.test.ts @@ -0,0 +1,393 @@ +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([]); + }); + + 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 identity match 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'); + }); + + 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'); + }); + + 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..c1f9268 --- /dev/null +++ b/scripts/check-dco.ts @@ -0,0 +1,335 @@ +/** + * 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. + */ + +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. + * + * Both halves 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. + * + * The trailing class is `[ \t\r]*` and not `\s*`: under the `m` flag `\s` also + * matches a newline, so a greedy `\s*$` can run past the end of the trailer's + * own line and match a `$` further down. + */ +const SIGNOFF_SOURCE = String.raw`^Signed-off-by:[ \t]*([^<>]+?)[ \t]*<([^<>]*)>[ \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 both halves of an identity. */ +function matchesIdentity(signoff: Signoff, identity: Identity): boolean { + return ( + signoff.name.trim().toLowerCase() === identity.name.trim().toLowerCase() && + 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 *identity* 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. Presence is still checked, which is strictly stronger. + * + * Merge commits are exempt: their content is certified by the commits they + * merge, and the person who ran `git merge` authored none of it. + */ +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; + } + + if (isBotIdentity(commit.author)) { + exemptions.push({ + sha, + subject, + author, + reason: 'bot', + detail: `signed by the app account as ${render(signoffs[0] as Signoff)}`, + }); + continue; + } + + const matched = signoffs.some( + (signoff) => matchesIdentity(signoff, commit.author) || matchesIdentity(signoff, commit.committer), + ); + + if (matched) { + signed += 1; + continue; + } + + const got = signoffs.map((signoff) => `"${render(signoff)}"`).join(', '); + violations.push({ + sha, + subject, + author, + detail: `expected a sign-off by "${author}", 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[] { + const stdout = execFileSync('git', ['log', '-z', `--format=${GIT_FORMAT}`, range], { + cwd, + encoding: 'utf8', + maxBuffer: 64 * 1024 * 1024, + }); + 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 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 " matching its author.\n` + + `Sign the whole branch and force-push:\n` + + ` git rebase --signoff ${range.split('..')[0]}\n` + + `See CONTRIBUTING.md and https://developercertificate.org/.`, + ); + } + + 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); + } +} From 05a6a7d9d726d79d72ae0bf0c69b873a4b1278cf Mon Sep 17 00:00:00 2001 From: Mark Beacom Date: Wed, 12 Aug 2026 10:13:22 -0400 Subject: [PATCH 2/4] fix(dco): require a bot's trailer to name it, and name both identities on failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects found in review, both observed failing before the fix. The bot exemption only checked that some sign-off existed. A bot-authored commit carrying nothing but an unrelated person's Signed-off-by was exempted and then reported as "signed by the app account as " — a report asserting something it had never checked, which is the ADR-0016 failure the exemption exists inside. Only the address half needs exempting: Dependabot authors as dependabot[bot] with a noreply address and signs as dependabot[bot] , so the name matches and only the address cannot. The trailer must now name the bot, and a commit whose signatures name nobody relevant falls through to the ordinary identity match so it fails with a precise message instead of being waved through. The failure diagnostic said "expected a sign-off by " while the classifier accepts the author or the committer. Where those differ, the message omitted a valid way to fix the commit; it now names both, and still names one where they are the same. Verified against the real Dependabot commit on PR #99, which the tightened rule still exempts, so no false positive was introduced. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom --- CHANGELOG.md | 6 ++-- CONTRIBUTING.md | 4 +-- scripts/check-dco.test.ts | 63 +++++++++++++++++++++++++++++++++++++-- scripts/check-dco.ts | 52 +++++++++++++++++++++++--------- 4 files changed, 103 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 28f9006..eef9574 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,10 +27,10 @@ Until `1.0.0`, minor releases may include breaking changes 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 identity match only, + 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, but presence is still checked. Every exemption is named in the job output, - so a commit is never skipped silently. + 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 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e27d95c..35d4155 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -64,8 +64,8 @@ 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 identity match -only, because they sign from a service address; they still have to sign. +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. ## Two hard rules diff --git a/scripts/check-dco.test.ts b/scripts/check-dco.test.ts index a07efbb..7ae4848 100644 --- a/scripts/check-dco.test.ts +++ b/scripts/check-dco.test.ts @@ -182,7 +182,6 @@ describe('findDcoViolations', () => { '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 @@ -224,8 +223,8 @@ describe('findDcoViolations', () => { }); // Negative case for the one place this is stricter than the DCO app, which - // skips app-authored commits outright. Exempting the identity match must not - // silently exempt presence too. + // 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' }), @@ -235,6 +234,64 @@ describe('findDcoViolations', () => { 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) }), diff --git a/scripts/check-dco.ts b/scripts/check-dco.ts index c1f9268..a8935fb 100644 --- a/scripts/check-dco.ts +++ b/scripts/check-dco.ts @@ -112,10 +112,15 @@ export function parseSignoffs(message: string): Signoff[] { 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 ( - signoff.name.trim().toLowerCase() === identity.name.trim().toLowerCase() && + sameName(signoff, identity) && signoff.email.trim().toLowerCase() === identity.email.trim().toLowerCase() ); } @@ -157,11 +162,13 @@ function render(identity: Identity): string { * 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 *identity* match only, because a bot signs - * from a service address — Dependabot's author is + * 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. Presence is still checked, which is strictly stronger. + * 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: their content is certified by the commits they * merge, and the person who ran `git merge` authored none of it. @@ -192,15 +199,27 @@ export function findDcoViolations(commits: readonly Commit[]): DcoReport { 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)) { - exemptions.push({ - sha, - subject, - author, - reason: 'bot', - detail: `signed by the app account as ${render(signoffs[0] as Signoff)}`, - }); - continue; + 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( @@ -212,12 +231,17 @@ export function findDcoViolations(commits: readonly Commit[]): DcoReport { 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 "${author}", but got ${got}`, + detail: `expected a sign-off by ${expected}, but got ${got}`, }); } @@ -313,7 +337,7 @@ function main(argv: readonly string[]): void { .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 " matching its author.\n` + + `Every commit needs "Signed-off-by: Your Name " naming its author or its committer.\n` + `Sign the whole branch and force-push:\n` + ` git rebase --signoff ${range.split('..')[0]}\n` + `See CONTRIBUTING.md and https://developercertificate.org/.`, From b08786664249cad1a0b816d342b8784c5180b0a2 Mon Sep 17 00:00:00 2001 From: Mark Beacom Date: Wed, 12 Aug 2026 10:14:48 -0400 Subject: [PATCH 3/4] docs(dco): record the PR-checkout limitation the gate cannot close alone MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A pull request runs this gate from its own merge checkout, so a change that edits the script or the workflow step invoking it can produce a green dco status over unsigned commits. Every gate in this repository shares the property, measured on #98: pull_request workflows execute the pull request's ci.yml, not main's. Recorded rather than papered over. Running the script from a trusted base revision leaves the invoking step equally under the pull request's control, so it would look like a control without being one — the failure ADR-0016 is about. Tracked repository-wide in #137, where the option that changes anything is a required review rather than a change to this file. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom --- scripts/check-dco.ts | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/scripts/check-dco.ts b/scripts/check-dco.ts index a8935fb..f3af0df 100644 --- a/scripts/check-dco.ts +++ b/scripts/check-dco.ts @@ -17,6 +17,16 @@ * * `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'; From 790fdeefc7cdc1299a140fa404f16976d8375c79 Mon Sep 17 00:00:00 2001 From: Mark Beacom Date: Wed, 12 Aug 2026 10:30:45 -0400 Subject: [PATCH 4/4] fix(dco): stop a bare Signed-off-by label from swallowing the real trailer A four-lens deep review found one defect, and it fails in the expensive direction: a correctly signed commit was reported as unsigned, which on a required check blocks the merge. The name and address classes excluded < and > but not newline, so a line reading exactly "Signed-off-by:" let the lazy name group run down the message to the next < and absorb the valid trailer beneath it into one match named "Signed-off-by: Jane Doe". That falsified this parser's own documented invariant, which claimed an extra match could only ever add a candidate and never remove a valid one. Confining every class to a single physical line fixes it and caps the backtracking as a side effect: a 500k-character bracket-free line went from superlinear rescanning to 1.4ms. Observed failing in a real repository before the fix, and the four new cases were watched failing against the old regex. The review's other accepted findings, none of which changed behaviour: Failure messages now distinguish "could not examine" from "found unsigned commits". An unfetched base made git say "fatal: bad revision", which reads as a defect in the pull request rather than in the checkout; the script now says which it is. This is the same reporting rule ADR-0016 states, applied to the check's own failures rather than to its verdicts. The remediation block now shows the push, not just the rebase, and says to use --force-with-lease rather than --force, since the tool has just rewritten history and the obvious next command is the destructive one. It also explains in one line what sign-off certifies, for a contributor who has never met a DCO. CI invokes the check:dco alias like every sibling gate rather than holding a second reference to the script path. Verified to work with no node_modules, since the job deliberately runs no bun install. ADR-0006 gains a dco-signoff-required assertion in frontmatter, mirroring ADR-0007's clean-clone-builds. In a project about governance-as-code, a control recorded only in prose is invisible to anything querying the corpus. The merge exemption's comment no longer claims more than it can: an evil merge's own diff is not certified by its parents. Matching the DCO app's skip is a deliberate contract choice, not a property of merges. CONTRIBUTING warns browser editors before the wall rather than after, since the first-PR section steers newcomers toward exactly the docs edits made in the web UI, which cannot carry a trailer. The PR template's checklist item points at that section for commits already made. Also corrects a contradiction inside this same Unreleased block, where the ratification entry still said in the present tense that no ruleset check enforces the DCO. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Signed-off-by: Mark Beacom --- .github/PULL_REQUEST_TEMPLATE.md | 4 +- .github/workflows/ci.yml | 2 +- CHANGELOG.md | 6 +- CONTRIBUTING.md | 6 ++ ...06-license-apache-2-and-single-monorepo.md | 11 ++++ scripts/check-dco.test.ts | 42 ++++++++++++ scripts/check-dco.ts | 64 ++++++++++++++----- 7 files changed, 114 insertions(+), 21 deletions(-) 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 2be9038..51e5711 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -172,4 +172,4 @@ jobs: # 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 scripts/check-dco.ts "$BASE_SHA..$HEAD_SHA" + run: bun run check:dco "$BASE_SHA..$HEAD_SHA" diff --git a/CHANGELOG.md b/CHANGELOG.md index eef9574..01e8c4c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -108,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 35d4155..1e9cbac 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -67,6 +67,12 @@ commit's author or committer exactly. Merge commits are exempt — the commits t 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/docs/adr/0006-license-apache-2-and-single-monorepo.md b/docs/adr/0006-license-apache-2-and-single-monorepo.md index fba7f62..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" diff --git a/scripts/check-dco.test.ts b/scripts/check-dco.test.ts index 7ae4848..311d4d6 100644 --- a/scripts/check-dco.test.ts +++ b/scripts/check-dco.test.ts @@ -92,6 +92,35 @@ describe('parseSignoffs', () => { ).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([]); }); @@ -414,6 +443,19 @@ describe('check-dco end to end', () => { 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}>` }, diff --git a/scripts/check-dco.ts b/scripts/check-dco.ts index f3af0df..6cee963 100644 --- a/scripts/check-dco.ts +++ b/scripts/check-dco.ts @@ -87,18 +87,23 @@ export interface DcoReport { * Anchored to the start of a physical line, because a sign-off is a trailer and * a mid-line mention is prose. * - * Both halves exclude `<` and `>`, which git's own ident parser forbids. The - * reference implementation captures the name greedily as `(.*)`, so on + * 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. - * - * The trailing class is `[ \t\r]*` and not `\s*`: under the `m` flag `\s` also - * matches a newline, so a greedy `\s*$` can run past the end of the trailer's - * own line and match a `$` further down. */ -const SIGNOFF_SOURCE = String.raw`^Signed-off-by:[ \t]*([^<>]+?)[ \t]*<([^<>]*)>[ \t\r]*$`; +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 { @@ -180,8 +185,11 @@ function render(identity: Identity): string { * accept an unrelated person's signature on a bot's commit and report it as * the app account's. * - * Merge commits are exempt: their content is certified by the commits they - * merge, and the person who ran `git merge` authored none of it. + * 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[] = []; @@ -302,11 +310,27 @@ export function parseGitLog(stdout: string): Commit[] { } export function readCommits(range: string, cwd?: string): Commit[] { - const stdout = execFileSync('git', ['log', '-z', `--format=${GIT_FORMAT}`, range], { - cwd, - encoding: 'utf8', - maxBuffer: 64 * 1024 * 1024, - }); + 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); } @@ -320,6 +344,7 @@ export function formatReport(range: string, report: DcoReport): string { 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 @@ -347,10 +372,15 @@ function main(argv: readonly string[]): void { .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 committer.\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 ${range.split('..')[0]}\n` + - `See CONTRIBUTING.md and https://developercertificate.org/.`, + ` 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.`, ); }