diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 32fae71..5a8ac57 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -2,7 +2,8 @@ The PR title must follow Conventional Commits (feat:, fix:, docs:, ci:, chore:, refactor:, test:, style:, perf:). CI checks this. -Every commit needs a DCO sign-off: `git commit -s`. See CONTRIBUTING.md. +Every commit author and declared co-author needs a matching DCO sign-off. +Use `git commit -s`; bots follow the same rule. See CONTRIBUTING.md. --> ## What changed and why diff --git a/.github/workflows/dco-assistant.yml b/.github/workflows/dco-assistant.yml index 247b770..5cab920 100644 --- a/.github/workflows/dco-assistant.yml +++ b/.github/workflows/dco-assistant.yml @@ -1,5 +1,5 @@ -# Records Developer Certificate of Origin sign-off. A contributor who forgot -# `git commit -s` can sign by leaving a comment instead of rewriting history. +# Records one-time Developer Certificate of Origin agreement. Commit trailers +# are validated independently by dco-trailers.yml; a comment cannot replace them. # # This uses pull_request_target because it must comment on and store # signatures for pull requests from forks, which the read-only token on a diff --git a/.github/workflows/dco-trailers.yml b/.github/workflows/dco-trailers.yml new file mode 100644 index 0000000..8d081a6 --- /dev/null +++ b/.github/workflows/dco-trailers.yml @@ -0,0 +1,28 @@ +# Commit trailers and the DCO Assistant's one-time agreement are separate checks. +name: Validate DCO trailers + +on: + pull_request: + types: [opened, synchronize, reopened, ready_for_review, edited] + +permissions: + contents: read + +jobs: + dco-trailers: + name: DCO trailers + runs-on: ubuntu-latest + timeout-minutes: 5 + env: + BASE_SHA: ${{ github.event.pull_request.base.sha }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: ${{ github.event.pull_request.head.sha }} + fetch-depth: 0 + persist-credentials: false + - name: Fetch base commit + run: git fetch --no-tags origin "$BASE_SHA" + - name: Validate every PR commit + run: python3 scripts/check_commit_signoffs.py --base "$BASE_SHA" --head "$HEAD_SHA" diff --git a/.github/workflows/semantic-pull-requests.yml b/.github/workflows/semantic-pull-requests.yml index d80a89d..a00b64f 100644 --- a/.github/workflows/semantic-pull-requests.yml +++ b/.github/workflows/semantic-pull-requests.yml @@ -1,5 +1,5 @@ -# The PR title becomes the squashed commit subject, so this is what keeps the -# history on main parseable and the changelog derivable. +# Validate the PR title. It becomes the main-branch subject when maintainers +# squash merge; squash-only repository settings are a separate policy change. name: Validate PR title on: diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 4175b6f..f2d6b9d 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -5,6 +5,9 @@ # uv owns lock formatting; notebooks skip rewriting text hooks individually. exclude: '^_[^/]+/|^uv\.lock$' +default_install_hook_types: [pre-commit, commit-msg] +default_stages: [pre-commit] + repos: - repo: https://github.com/pre-commit/pre-commit-hooks rev: v5.0.0 @@ -47,7 +50,13 @@ repos: name: SPDX headers (staged files) entry: uv run python scripts/update_license_headers.py language: system - files: '^(src/|tests/|scripts/|templates/|Makefile$|conftest\.py$)' + files: '^(src/|tests/|tests_e2e/|scripts/|templates/|Makefile$|conftest\.py$)' + - id: dco-trailers + name: DCO author and co-author sign-offs + entry: python3 scripts/check_commit_signoffs.py --commit-msg + language: system + stages: [commit-msg] + always_run: true - id: ruff name: ruff (lint) entry: uv run ruff check --fix diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6e3b3e5..efd8a3d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -34,6 +34,23 @@ it, start with the [README](README.md). git rebase --signoff main ``` +Every commit requires an exact name and email match between its author and a +`Signed-off-by` trailer. Bots have no trailer exemption. Each declared +`Co-authored-by` identity also needs a matching sign-off; obtain their DCO +certification before adding it. Cryptographic commit signing is separate and +is not required by this repository. + +Install both local hooks after setting up the development environment: + +```bash +uv run pre-commit install --hook-type pre-commit --hook-type commit-msg +``` + +The commit-message hook checks the author identity provided by Git, including +`--author` overrides. CI independently checks every submitted commit. See +[contribution policy](docs/engineering/contribution-policy.md) for recovery, +stacked PRs, and the maintainer settings checklist. + ### Recording your signature, once The first pull request you open will get a comment from the DCO Assistant bot @@ -260,7 +277,9 @@ policy through `.github/CODEOWNERS`. Your PR title must follow [Conventional Commits](https://www.conventionalcommits.org/) (`feat:`, `fix:`, `docs:`, `ci:`, `chore:`, `refactor:`, `test:`, `style:`, -`perf:`), because it becomes the squashed commit subject on `main`. +`perf:`). Squash-only merging is the proposed policy. Until maintainers apply +the settings in the [contribution policy](docs/engineering/contribution-policy.md), +the title check alone does not guarantee the commit subject on `main`. ## What CI runs diff --git a/Makefile b/Makefile index 1f7ae98..ef79b35 100644 --- a/Makefile +++ b/Makefile @@ -87,14 +87,9 @@ check-lock: check-tools ## Check dependency metadata against uv.lock without re # Reinstalling the project restores any file that removing another distribution # took with it, such as the `usersim` command an earlier name also installed. -install-dev: check-tools ## Sync the workspace with dev tooling and install pre-commit hooks +install-dev: check-tools ## Sync development tools and install static and commit-message hooks $(UV) sync --locked --reinstall-package usersim - @if [ ! -f "$$(git rev-parse --git-path hooks/pre-commit)" ]; then \ - echo "Installing pre-commit hooks..."; \ - $(UV) run pre-commit install; \ - else \ - echo "Pre-commit hooks already installed"; \ - fi + $(UV) run pre-commit install --hook-type pre-commit --hook-type commit-msg # --locked installs strictly from uv.lock and refuses to re-resolve, so # dependency drift becomes a build failure rather than a silent diff --git a/docs/engineering/contribution-policy.md b/docs/engineering/contribution-policy.md new file mode 100644 index 0000000..8fc2d46 --- /dev/null +++ b/docs/engineering/contribution-policy.md @@ -0,0 +1,84 @@ +# Contribution checks and merge policy + +Every commit submitted in a pull request needs a `Signed-off-by: Name ` +trailer matching its author name and email exactly. Each `Co-authored-by` +identity also needs its own matching sign-off. Do not add another person's +certification without their agreement. Bots follow the same trailer rule; +there is no bot allowlist in the trailer checker. + +The DCO Assistant separately records one-time contributor agreement. Its +existing `dependabot[bot]` agreement exemption does not exempt bot commits from +trailers. A comment to the Assistant cannot repair an unsigned commit. +Cryptographic signatures are independent of DCO trailers and are not required. + +## Run checks locally + +`make install-dev` installs both Git hooks, including when only the static hook +was installed previously. To reinstall them directly: + +```bash +uv run pre-commit install --hook-type pre-commit --hook-type commit-msg +``` + +Normal commits use `git commit -s`. The commit-message hook invokes +`scripts/check_commit_signoffs.py` with Git's author identity. The PR workflow +uses the same checker against stored commit authors, independently of whether +local hooks were installed. Git parses the final trailer block; text mentioning +a sign-off in an earlier paragraph does not count. + +To reproduce the PR check, supply the actual parent base and head SHAs: + +```bash +python3 scripts/check_commit_signoffs.py --base "$(git rev-parse origin/main)" --head "$(git rev-parse HEAD)" +``` + +For a stacked PR, replace `origin/main` with its parent branch. The checker +validates every commit reachable from the head but not the base, including +merge commits. An empty range or incomplete history fails. Shallow repositories +are rejected even when both endpoint SHAs are present; run `git fetch --unshallow` +before checking a range. Fetch the parent first if it is missing locally. +Retargeting a stacked PR reruns the trailer workflow against its new base, even +when the head commit is unchanged. + +If the latest commit is yours and its author matches your Git identity: + +```bash +git commit --amend --no-edit -s +``` + +For several commits authored by you, rebase onto the actual PR parent: + +```bash +git rebase --signoff origin/main +``` + +Review the rewritten commits with the checker before updating a published +branch. Coordinate history changes with collaborators. If author and committer +differ, `-s` adds the committer's identity; obtain the author's certification +and add that exact trailer as well. Preserve co-author trailers and their +certifications during rewrites. + +## Maintainer settings checklist + +The new workflow publishes the separate `DCO trailers` job status. Adding this +file does not configure repository rules. Before treating trailer validation +as enforced, maintainers must: + +- Add `DCO trailers` as a required check for `main`, using a successful workflow + run to identify the exact status context. +- Retain the existing DCO Assistant and PR-title requirements. Migrating checks + from `ci.yml` to its aggregate status must not remove these separate checks. +- Review bot-generated PRs and configure their authorship/sign-offs to satisfy + the same rule before making the new status required. +- Verify CODEOWNERS review requirements and the required checks in repository + rules after each status migration. + +Squash-only merging is the proposed repository policy so the validated PR +title becomes the subject on `main`. This change does not alter GitHub merge +settings. Maintainers must inspect current settings, enable squash merging, +and disable merge commits and rebase merging to adopt that policy. Until +then, the PR-title check validates the PR title without guaranteeing the final +commit subject. If merge or rebase remains allowed, document that choice and +add commit-subject validation before claiming conventional main-branch history +is enforced. When squash merging, preserve author and co-author certifications +in the final commit message and ensure its author has a matching sign-off. diff --git a/scripts/check_commit_signoffs.py b/scripts/check_commit_signoffs.py new file mode 100644 index 0000000..3c51009 --- /dev/null +++ b/scripts/check_commit_signoffs.py @@ -0,0 +1,91 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Validate author and co-author DCO trailers in Git commits or a hook message.""" + +from __future__ import annotations + +import argparse +import re +import subprocess +import sys +from pathlib import Path + +IDENTITY = re.compile(r"[^<>\n]+ <[^<>\s@]+@[^<>\s@]+>") +REVISION = re.compile(r"[0-9a-f]{40}(?:[0-9a-f]{24})?") + + +def git(*args: str, input_text: str | None = None) -> str: + return subprocess.run( + ["git", *args], + input=input_text, + text=True, + capture_output=True, + check=True, + ).stdout.strip() + + +def check_message(message: str, author: str) -> list[str]: + """Use Git's trailer parser so a sign-off embedded in prose does not count.""" + trailers = git("-c", "trailer.separators=:", "interpret-trailers", "--parse", input_text=message) + signoffs = set() + required = {author} + errors = [] + for line in trailers.splitlines(): + key, _, identity = line.partition(":") + if key.lower() not in {"signed-off-by", "co-authored-by"}: + continue + identity = identity.strip() + if not IDENTITY.fullmatch(identity): + errors.append(f"malformed {key}: {identity!r}") + elif key.lower() == "signed-off-by": + signoffs.add(identity) + else: + required.add(identity) + for identity in sorted(required - signoffs): + errors.append(f"missing Signed-off-by: {identity}") + return errors + + +def check_range(base: str, head: str) -> list[str]: + if not REVISION.fullmatch(base) or not REVISION.fullmatch(head): + raise ValueError("--base and --head must be full commit SHAs") + if git("rev-parse", "--is-shallow-repository") == "true": + raise ValueError("range validation requires complete history; run git fetch --unshallow first") + commits = git("rev-list", "--reverse", f"{base}..{head}").splitlines() + if not commits: + return ["no commits found in the requested range"] + errors = [] + for commit in commits: + author = git("show", "-s", "--format=%an <%ae>", commit) + message = git("show", "-s", "--format=%B", commit) + errors.extend(f"{commit}: {error}" for error in check_message(message, author)) + return errors + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--commit-msg", type=Path, help="validate a commit-msg hook file") + parser.add_argument("--base", help="PR base commit SHA") + parser.add_argument("--head", help="PR head commit SHA") + args = parser.parse_args() + if bool(args.commit_msg) == bool(args.base or args.head) or bool(args.base) != bool(args.head): + parser.error("provide --commit-msg FILE or both --base SHA and --head SHA") + try: + if args.commit_msg: + author = git("var", "GIT_AUTHOR_IDENT").rsplit(">", 1)[0] + ">" + errors = check_message(args.commit_msg.read_text(encoding="utf-8"), author) + else: + errors = check_range(args.base, args.head) + except (OSError, UnicodeError, ValueError, subprocess.CalledProcessError) as exc: + print(f"DCO validation could not complete: {exc}", file=sys.stderr) + return 1 + for error in errors: + print(error, file=sys.stderr) + if errors: + print("Use git commit -s with the commit author's identity; see CONTRIBUTING.md.", file=sys.stderr) + return int(bool(errors)) + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tests/test_ci_workflows.py b/tests/test_ci_workflows.py index 8d1099d..42f9722 100644 --- a/tests/test_ci_workflows.py +++ b/tests/test_ci_workflows.py @@ -353,3 +353,14 @@ def test_native_matrix_workers_and_diagnostics_are_bounded(self) -> None: and step.get("uses", "").startswith("actions/upload-artifact@") for step in doc["jobs"][name]["steps"] ), name + + +def test_dco_trailers_rechecks_range_after_retargeting() -> None: + doc = _load(_WORKFLOWS / "dco-trailers.yml") + trigger = doc.get("on", doc.get(True))["pull_request"] + assert "edited" in trigger["types"] + assert "branches" not in trigger + assert doc["permissions"] == {"contents": "read"} + env = doc["jobs"]["dco-trailers"]["env"] + assert env["BASE_SHA"] == "${{ github.event.pull_request.base.sha }}" + assert env["HEAD_SHA"] == "${{ github.event.pull_request.head.sha }}" diff --git a/tests/test_commit_signoffs.py b/tests/test_commit_signoffs.py new file mode 100644 index 0000000..240e7ec --- /dev/null +++ b/tests/test_commit_signoffs.py @@ -0,0 +1,177 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 + +"""Exercise commit policy against real temporary Git histories.""" + +from __future__ import annotations + +import os +import subprocess +import sys +from pathlib import Path + +import pytest + +SCRIPT = Path(__file__).resolve().parents[1] / "scripts/check_commit_signoffs.py" +AUTHOR = "Example Author " + + +def git(repo, *args, **kwargs): + return subprocess.run( + ["git", "-C", str(repo), *args], check=True, text=True, capture_output=True, **kwargs + ).stdout.strip() + + +@pytest.fixture +def repo(tmp_path, monkeypatch): + monkeypatch.setenv("GIT_CONFIG_GLOBAL", os.devnull) + monkeypatch.setenv("GIT_CONFIG_NOSYSTEM", "1") + for name in ("GIT_AUTHOR_NAME", "GIT_AUTHOR_EMAIL", "GIT_COMMITTER_NAME", "GIT_COMMITTER_EMAIL"): + monkeypatch.delenv(name, raising=False) + git(tmp_path, "init") + git(tmp_path, "config", "user.name", "Example Author") + git(tmp_path, "config", "user.email", "author@example.com") + git(tmp_path, "-c", "commit.gpgsign=false", "commit", "--allow-empty", "-m", "base") + return tmp_path + + +def run_check(repo, *args): + return subprocess.run([sys.executable, str(SCRIPT), *args], cwd=repo, capture_output=True, text=True) + + +@pytest.mark.parametrize( + "trailers,valid", + [ + (f"Signed-off-by: {AUTHOR}", True), + ("", False), + ("Signed-off-by: Someone Else ", False), + ("Signed-off-by: Example Author ", False), + ("Signed-off-by: Example Author author@example.com", False), + (f"Signed-off-by: {AUTHOR}\nCo-authored-by: Other ", False), + ( + f"Signed-off-by: {AUTHOR}\nCo-authored-by: Other \nSigned-off-by: Other ", + True, + ), + (f"Signed-off-by: {AUTHOR}\nCo-authored-by: malformed", False), + ], +) +def test_hook_validates_identity_and_coauthors(repo, trailers, valid): + message = repo / "message" + message.write_text(f"feat: example\n\n{trailers}\n") + result = run_check(repo, "--commit-msg", str(message)) + assert (result.returncode == 0) == valid, result.stderr + + +def test_signoff_in_prose_is_not_a_trailer(repo): + message = repo / "message" + message.write_text(f"feat: example\n\nSigned-off-by: {AUTHOR}\n\nThis is prose, not a trailer block.\n") + assert run_check(repo, "--commit-msg", str(message)).returncode == 1 + + +def test_each_commit_is_checked_including_bots(repo): + base = git(repo, "rev-parse", "HEAD") + git(repo, "-c", "commit.gpgsign=false", "commit", "--allow-empty", "-m", f"first\n\nSigned-off-by: {AUTHOR}") + bot = "dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>" + git(repo, "-c", "commit.gpgsign=false", "commit", "--allow-empty", "--author", bot, "-m", "unsigned bot") + head = git(repo, "rev-parse", "HEAD") + result = run_check(repo, "--base", base, "--head", head) + assert result.returncode == 1 + assert head in result.stderr + git( + repo, + "-c", + "commit.gpgsign=false", + "commit", + "--amend", + "--allow-empty", + "-m", + f"signed bot\n\nSigned-off-by: {bot}", + ) + assert run_check(repo, "--base", base, "--head", git(repo, "rev-parse", "HEAD")).returncode == 0 + + +def test_hook_uses_author_environment(repo): + message = repo / "message" + message.write_text("feat: example\n\nSigned-off-by: Other \n") + result = subprocess.run( + [sys.executable, str(SCRIPT), "--commit-msg", str(message)], + cwd=repo, + env={**os.environ, "GIT_AUTHOR_NAME": "Other", "GIT_AUTHOR_EMAIL": "other@example.com"}, + capture_output=True, + ) + assert result.returncode == 0 + + +def test_empty_range_and_invalid_revision_fail(repo): + head = git(repo, "rev-parse", "HEAD") + assert run_check(repo, "--base", head, "--head", head).returncode == 1 + assert run_check(repo, "--base", "--all", "--head", head).returncode != 0 + + +def test_real_commit_hook_receives_author_override(repo): + import shlex + + hook = repo / ".git/hooks/commit-msg" + hook.write_text(f'#!/bin/sh\nexec {shlex.quote(sys.executable)} {shlex.quote(str(SCRIPT))} --commit-msg "$1"\n') + hook.chmod(0o755) + other = "Other Author " + git( + repo, + "-c", + "commit.gpgsign=false", + "commit", + "--allow-empty", + "--author", + other, + "-m", + f"other\n\nSigned-off-by: {other}", + ) + with pytest.raises(subprocess.CalledProcessError): + git( + repo, + "-c", + "commit.gpgsign=false", + "commit", + "--allow-empty", + "--author", + other, + "-s", + "-m", + "wrong identity", + ) + + +def test_missing_history_fails_closed(repo): + head = git(repo, "rev-parse", "HEAD") + result = run_check(repo, "--base", "0" * 40, "--head", head) + assert result.returncode == 1 + assert "could not complete" in result.stderr + + +@pytest.mark.parametrize("signed_intermediate", [False, True]) +def test_shallow_history_cannot_hide_intermediate_commits(repo, signed_intermediate): + base = git(repo, "rev-parse", "HEAD") + message = "intermediate" + if signed_intermediate: + message += f"\n\nSigned-off-by: {AUTHOR}" + git(repo, "-c", "commit.gpgsign=false", "commit", "--allow-empty", "-m", message) + intermediate = git(repo, "rev-parse", "HEAD") + git(repo, "-c", "commit.gpgsign=false", "commit", "--allow-empty", "-m", f"tip\n\nSigned-off-by: {AUTHOR}") + head = git(repo, "rev-parse", "HEAD") + + complete = run_check(repo, "--base", base, "--head", head) + assert complete.returncode == (0 if signed_intermediate else 1), complete.stderr + if not signed_intermediate: + assert intermediate in complete.stderr + + shallow = repo / "shallow" + git(repo, "clone", "--depth=1", repo.as_uri(), str(shallow)) + git(shallow, "fetch", "--depth=1", "origin", base) + result = run_check(shallow, "--base", base, "--head", head) + assert result.returncode == 1 + assert "complete history" in result.stderr + assert "git fetch --unshallow" in result.stderr + + git(shallow, "fetch", "--unshallow", "origin") + recovered = run_check(shallow, "--base", base, "--head", head) + assert recovered.returncode == complete.returncode, recovered.stderr