diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 9131aa5..fda3cdb 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -34,6 +34,9 @@ $ uv run pytest -q ## Checklist +- [ ] My PR title and every introduced commit follow the + [commit policy](https://github.com/BUAA-CI-LAB/EmbodiRun/blob/main/CONTRIBUTING.md#commit-messages-and-sign-off), + and every author/coauthor has supplied their own DCO `Signed-off-by`. - [ ] I updated [`docs/en/support-matrix.md`](https://github.com/BUAA-CI-LAB/EmbodiRun/blob/main/docs/en/support-matrix.md) for any new combination, with versions, configuration, hardware, checkpoint, exact command, and observed result. diff --git a/.github/workflows/commit-policy.yml b/.github/workflows/commit-policy.yml new file mode 100644 index 0000000..a0d1e33 --- /dev/null +++ b/.github/workflows/commit-policy.yml @@ -0,0 +1,29 @@ +name: Commit policy + +on: + pull_request: + types: [opened, synchronize, reopened, edited, ready_for_review] + +permissions: + contents: read + +concurrency: + group: commit-policy-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + commits: + name: Commit policy + runs-on: ubuntu-24.04 + timeout-minutes: 5 + steps: + - name: Checkout the merge tree and full commit history + uses: actions/checkout@v7 + with: + fetch-depth: 0 + persist-credentials: false + + # Check the event's actual head, not GitHub's unsigned test-merge commit. + # Read the title as JSON data instead of interpolating it into shell code. + - name: Check all introduced commits and the PR title + run: python3 -I scripts/check_commit_policy.py --event "$GITHUB_EVENT_PATH" diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index bc13176..8b6b1f0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -22,6 +22,114 @@ the same license, as described in section 5 of the license. Do not contribute code you cannot license this way, and do not paste third-party code without its license and attribution. +## Commit messages and sign-off + +Use [Conventional Commits 1.0](https://www.conventionalcommits.org/en/v1.0.0/) +and certify every contribution under the +[Developer Certificate of Origin (DCO) 1.1](https://developercertificate.org/). +The `Commit policy` CI check validates **every commit introduced by a PR**, +including contributor-created merge commits, and the PR title. Existing base +branch history and GitHub's temporary test-merge commit are outside that range. + +### Subject and categories + +```text +[(scope)][!]: +``` + +| Type | Use for | +| --- | --- | +| `feat` | A new user-facing capability | +| `fix` | A bug fix | +| `docs` | Documentation changes | +| `style` | Formatting changes without behavior changes | +| `refactor` | Restructuring without a new feature or bug fix | +| `perf` | Performance improvements | +| `test` | Adding or correcting meaningful tests | +| `build` | Dependencies, packaging, or build tooling | +| `ci` | CI workflows and their tooling | +| `chore` | Other repository maintenance | +| `revert` | Reverting a change; identify the original commit in the body | + +The following subject rules also apply to the PR title: + +- Use a lowercase type from the table, followed by exactly `: `. +- An optional lowercase scope starts with a letter or digit and contains only + letters, digits, `.`, `_`, `/`, or `-`. Prefer the affected domain, such as + `deployment`, `devices`, or `model_services`. +- Keep the complete subject within 72 characters, including the prefix. Use a + specific description, without surrounding whitespace, control characters, + or a final `.` or `。`. Prefer an imperative phrase such as "validate device + configuration"; Chinese descriptions are also accepted. +- Keep each commit focused on one logical change. Mark an incompatible change + with `!` before the colon, or a `BREAKING CHANGE: ` footer. + +Separate the subject, optional body, and final trailer block with empty lines. +Use the body to explain why the change is needed and any migration steps; +wrapping prose at about 72 characters is recommended, but long URLs and code +are allowed. No message line may have trailing whitespace. + +```text +fix(deployment): validate device configuration before launch + +Report missing device configuration before starting remote processes. + +Signed-off-by: Your Name +``` + +### Required DCO sign-off + +Every introduced commit must end with a trailer spelled exactly +`Signed-off-by: Name `. The name must match the commit's author name, +and the email must match its author email (case-insensitively). A GitHub +`noreply` address is accepted. Each contributor named in a `Co-authored-by` +trailer must also provide a matching `Signed-off-by` trailer. A sign-off in +the middle of the message, or one from somebody other than the author, does +not satisfy this requirement. + +Read the DCO before signing. Configure your own Git identity and use `-s` +when creating a commit: + +```bash +git config user.name "Your Name" +git config user.email "you@example.com" +git commit -s -m "fix(deployment): validate device configuration before launch" +``` + +`-s` adds the committer's DCO trailer; it is distinct from `-S`, which creates +a cryptographic signature. When committing another author's work, obtain +their sign-off rather than signing on their behalf. Coauthors must each +certify their own contribution. + +To repair your latest unpublished or PR-branch commit, use +`git commit --amend --signoff`; edit its subject if necessary. For older +commits, use `git rebase -i origin/main`, stop at each of your affected commits, +amend it, and continue the rebase. After rewriting your own published PR +branch, push with `git push --force-with-lease`. Do not rewrite merged history +or add other contributors' sign-offs yourself. + +If merging main into your branch, supply a compliant subject and your sign-off: + +```bash +git merge --signoff -m "chore(merge): sync main" origin/main +``` + +Maintainers must retain the authors' and coauthors' sign-offs when squashing +or rebasing. Use a compliant final subject and add your own sign-off when +creating a final merge commit; the PR check does not inspect the commit that +GitHub creates after merging. + +### Check locally + +```bash +git fetch origin main +python3 scripts/check_commit_policy.py --base origin/main --head HEAD \ + --title "fix(deployment): validate device configuration before launch" +``` + +CI reports the failing commit IDs and individual rules. PR edits rerun +this lightweight check without rerunning the CPU and documentation workflows. + ## Development setup ```bash diff --git a/docs/en/contributing.md b/docs/en/contributing.md index 431633c..5406f2e 100644 --- a/docs/en/contributing.md +++ b/docs/en/contributing.md @@ -7,6 +7,25 @@ The full contribution guide is [`CONTRIBUTING.md`](https://github.com/BUAA-CI-LAB/EmbodiRun/blob/main/CONTRIBUTING.md) in the repository root. Start here for development setup and pull request checks. +## Commit messages and sign-off + +Use `[(scope)][!]: ` subjects, with a lowercase type from +`feat`, `fix`, `docs`, `style`, `refactor`, `perf`, `test`, `build`, `ci`, +`chore`, or `revert`, and a maximum of 72 characters. The PR title follows +the same convention. Every introduced commit must have a final +`Signed-off-by: Name ` trailer matching its author; named coauthors +must supply their own sign-offs too. + +```bash +git commit -s -m "fix(deployment): validate device configuration before launch" +python3 scripts/check_commit_policy.py --base origin/main --head HEAD +``` + +The `Commit policy` CI check validates all new PR commits and the PR title. +Read the +[complete policy and DCO instructions](https://github.com/BUAA-CI-LAB/EmbodiRun/blob/main/CONTRIBUTING.md#commit-messages-and-sign-off) +for category definitions, message layout, breaking changes, and repair commands. + ## Development setup ```bash diff --git a/scripts/check_commit_policy.py b/scripts/check_commit_policy.py new file mode 100644 index 0000000..109f406 --- /dev/null +++ b/scripts/check_commit_policy.py @@ -0,0 +1,132 @@ +"""Check Conventional Commit subjects and author DCO sign-offs in a Git range.""" + +from __future__ import annotations + +import argparse +import json +import re +import subprocess +import sys +from pathlib import Path + +TYPES = ("feat", "fix", "docs", "style", "refactor", "perf", "test", "build", "ci", "chore", "revert") +SUBJECT = re.compile(r"([a-z]+)(?:\(([a-z0-9][a-z0-9._/-]*)\))?(!)?: ([^\s].*)") +IDENTITY = re.compile(r"([^<>\r\n]+) <([^<>\s@]+@[^<>\s@]+)>") + + +def git(repo: Path, *args: str, message: str | None = None) -> str: + result = subprocess.run( + ["git", "-C", str(repo), *args], + input=message, + capture_output=True, + text=True, + encoding="utf-8", + errors="replace", + check=True, + ) + return result.stdout + + +def subject_errors(subject: str) -> list[str]: + errors = [] + match = SUBJECT.fullmatch(subject) + if match is None or match[1] not in TYPES: + errors.append("use [(scope)][!]: ; types: " + ", ".join(TYPES)) + if len(subject) > 72: + errors.append("subject must be at most 72 characters, including type and scope") + if subject != subject.strip() or any(ord(char) < 32 for char in subject): + errors.append("subject must be one line without control characters or surrounding whitespace") + if subject.endswith((".", "。")): + errors.append("subject must not end with a full stop") + return errors + + +def commit_errors(repo: Path, message: str, author_name: str, author_email: str) -> list[str]: + lines = message.splitlines() + errors = subject_errors(lines[0] if lines else "") + if len(lines) > 1 and lines[1] != "": + errors.append("separate the subject from the body/trailers with an empty line") + if any(line != line.rstrip() for line in lines): + errors.append("commit message lines must not have trailing whitespace") + + trailers = git(repo, "interpret-trailers", "--parse", "--no-divider", message=message) + identities = set() + coauthors = set() + for line in trailers.splitlines(): + key, _, value = line.partition(": ") + if key.lower() not in {"signed-off-by", "co-authored-by"}: + continue + identity = IDENTITY.fullmatch(value) + if identity is None: + errors.append(f"{key} must have the form Name ") + continue + name, email = identity.groups() + if key.lower() == "signed-off-by": + if key != "Signed-off-by": + errors.append("use the exact trailer spelling Signed-off-by") + identities.add((name, email.lower())) + else: + coauthors.add((name, email.lower())) + + for name, email in sorted({(author_name, author_email.lower()), *coauthors}): + if (name, email) not in identities: + errors.append(f"missing author sign-off in the final trailers: Signed-off-by: {name} <{email}>") + return errors + + +def check_range(repo: Path, base: str, head: str, *, title: str | None = None) -> list[tuple[str, list[str]]]: + """Read every introduced commit, including authored merges, without checking out code.""" + base = git(repo, "rev-parse", "--verify", "--end-of-options", f"{base}^{{commit}}").strip() + head = git(repo, "rev-parse", "--verify", "--end-of-options", f"{head}^{{commit}}").strip() + git(repo, "merge-base", base, head) + commits = git(repo, "rev-list", "--reverse", f"{base}..{head}").splitlines() + if not commits: + raise ValueError("the range contains no new commits; supply the PR base and actual head") + results = [] + if title is not None: + results.append(("PR title", subject_errors(title))) + for commit in commits: + name, email, message = git(repo, "show", "-s", "--format=%an%x00%ae%x00%B", commit).split("\0", 2) + results.append((commit, commit_errors(repo, message, name, email))) + return results + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--repo", type=Path, default=Path.cwd()) + parser.add_argument("--base", help="base revision, usually origin/main") + parser.add_argument("--head", help="actual contribution head, usually HEAD") + parser.add_argument("--title", help="also check the proposed PR/squash subject") + parser.add_argument("--event", type=Path, help="GitHub pull_request event JSON containing the base, head and title") + args = parser.parse_args(argv) + if args.event and any(value is not None for value in (args.base, args.head, args.title)): + parser.error("use --event or --base/--head/--title, not both") + if not args.event and (args.base is None or args.head is None): + parser.error("supply --base and --head, or --event") + try: + if args.event: + pr = json.loads(args.event.read_text(encoding="utf-8"))["pull_request"] + args.base, args.head, args.title = pr["base"]["sha"], pr["head"]["sha"], pr["title"] + if not all(isinstance(value, str) and value for value in (args.base, args.head, args.title)): + raise ValueError("pull_request base/head SHA and title must be nonempty strings") + results = check_range(args.repo, args.base, args.head, title=args.title) + except (OSError, ValueError, KeyError, TypeError, subprocess.CalledProcessError) as error: + print(f"Commit policy could not inspect the range: {error}", file=sys.stderr) + return 2 + failed = False + for label, errors in results: + if errors: + failed = True + print(f"FAIL {label}", file=sys.stderr) + for error in errors: + print(" " + error, file=sys.stderr) + if failed: + print("See CONTRIBUTING.md#commit-messages-and-sign-off for the rules and repair commands.", file=sys.stderr) + return 1 + count = sum(label != "PR title" for label, _ in results) + print(f"Commit policy passed for {count} commit(s)" + (" and the PR title." if args.title is not None else ".")) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tests/test_commit_policy.py b/tests/test_commit_policy.py new file mode 100644 index 0000000..543ad3d --- /dev/null +++ b/tests/test_commit_policy.py @@ -0,0 +1,177 @@ +"""Exercise the commit gate against real Git histories and event payloads.""" + +from __future__ import annotations + +import json +import os +import subprocess +import sys +from pathlib import Path + +import pytest + +SCRIPT = Path(__file__).resolve().parents[1] / "scripts" / "check_commit_policy.py" +SIGNOFF = "Signed-off-by: Test Author " + + +def git(repo: Path, *args: str, message: str | None = None) -> str: + env = { + **os.environ, + "GIT_CONFIG_GLOBAL": os.devnull, + "GIT_CONFIG_NOSYSTEM": "1", + "GIT_AUTHOR_NAME": "Test Author", + "GIT_AUTHOR_EMAIL": "author@example.com", + "GIT_COMMITTER_NAME": "Test Author", + "GIT_COMMITTER_EMAIL": "author@example.com", + } + return subprocess.run( + ["git", "-C", str(repo), "-c", "commit.gpgsign=false", "-c", f"core.hooksPath={os.devnull}", *args], + input=message, + capture_output=True, + text=True, + check=True, + env=env, + ).stdout.strip() + + +def commit(repo: Path, message: str) -> str: + git(repo, "commit", "--allow-empty", "--cleanup=verbatim", "-F", "-", message=message) + return git(repo, "rev-parse", "HEAD") + + +@pytest.fixture +def history(tmp_path: Path) -> tuple[Path, str]: + git(tmp_path, "init", "-q", "-b", "main") + # Adoption must not require rewriting unsigned legacy commits on main. + base = commit(tmp_path, "legacy initial commit") + return tmp_path, base + + +def check(repo: Path, base: str, *, title: str | None = None, head: str = "HEAD") -> subprocess.CompletedProcess[str]: + args = [sys.executable, "-I", str(SCRIPT), "--repo", str(repo), "--base", base, f"--head={head}"] + if title is not None: + args.extend(["--title", title]) + return subprocess.run(args, capture_output=True, text=True) + + +def test_valid_range_with_scope_breaking_change_and_body(history): + repo, base = history + commit(repo, f"feat(deployment): validate device configuration\n\n{SIGNOFF}\n") + commit( + repo, + "fix(model_services)!: require protocol version\n\n" + "Reject peers without an explicit version.\n\n" + f"BREAKING CHANGE: peers must send a protocol version.\n{SIGNOFF}\n", + ) + result = check(repo, base, title="feat(deployment): validate device configuration") + assert result.returncode == 0, result.stderr + assert "2 commit(s) and the PR title" in result.stdout + + +def test_unsigned_earlier_commit_is_not_hidden_by_signed_head(history): + repo, base = history + unsigned = commit(repo, "fix: correct device state\n") + signed = commit(repo, f"docs: explain device state\n\n{SIGNOFF}\n") + result = check(repo, base) + assert result.returncode == 1 + assert unsigned in result.stderr + assert signed not in result.stderr + assert "missing author sign-off" in result.stderr + + +@pytest.mark.parametrize( + ("message", "error"), + [ + ("fix: correct state\n\nSigned-off-by: Other Person \n", "missing author sign-off"), + (f"fix: correct state\n\n{SIGNOFF}\n\nThis is still message prose.\n", "missing author sign-off"), + ("fix: correct state\n\nSigned-off-by: Test Author\n", "must have the form"), + ("fix: correct state\n\nsigned-off-by: Test Author \n", "exact trailer spelling"), + ], +) +def test_signoff_must_be_a_valid_final_author_trailer(history, message, error): + repo, base = history + commit(repo, message) + result = check(repo, base) + assert result.returncode == 1 + assert error in result.stderr + + +@pytest.mark.parametrize("coauthor_signed", [False, True]) +def test_each_named_coauthor_requires_their_own_signoff(history, coauthor_signed): + repo, base = history + message = f"feat: coordinate device setup\n\nCo-authored-by: Second Author \n{SIGNOFF}\n" + if coauthor_signed: + message += "Signed-off-by: Second Author \n" + commit(repo, message) + result = check(repo, base) + assert result.returncode == (0 if coauthor_signed else 1), result.stderr + if not coauthor_signed: + assert "Second Author " in result.stderr + + +@pytest.mark.parametrize("merge_signed", [False, True]) +def test_authored_merges_checked_and_updated_base_history_excluded(history, merge_signed): + repo, _ = history + git(repo, "switch", "-c", "upstream") + base = commit(repo, "unsigned upstream change") + git(repo, "switch", "main") + commit(repo, f"feat: coordinate device setup\n\n{SIGNOFF}\n") + message = "chore(merge): sync main" + if merge_signed: + message += f"\n\n{SIGNOFF}" + git(repo, "merge", "--no-ff", "--cleanup=verbatim", "-m", message, "upstream") + merge_sha = git(repo, "rev-parse", "HEAD") + result = check(repo, base) + assert result.returncode == (0 if merge_signed else 1), result.stderr + if merge_signed: + assert "2 commit(s)" in result.stdout + else: + assert merge_sha in result.stderr + assert base not in result.stderr + + +@pytest.mark.parametrize( + ("message", "error"), + [ + (f"update: correct state\n\n{SIGNOFF}\n", "use "), + (f"fix: {'x' * 68}\n\n{SIGNOFF}\n", "at most 72 characters"), + (f"fix: correct state\nBody without an empty line.\n\n{SIGNOFF}\n", "empty line"), + (f"fix: correct state\n\nBody with trailing whitespace. \n\n{SIGNOFF}\n", "trailing whitespace"), + (f"fix: correct state.\n\n{SIGNOFF}\n", "full stop"), + ], +) +def test_message_rules_reject_invalid_real_commits(history, message, error): + repo, base = history + commit(repo, message) + result = check(repo, base) + assert result.returncode == 1 + assert error in result.stderr + + +@pytest.mark.parametrize("title_valid", [False, True]) +def test_event_uses_actual_head_and_treats_title_as_data(history, title_valid): + repo, base = history + head = commit(repo, f"ci: validate commit policy\n\n{SIGNOFF}\n") + # A temporary unsigned merge at HEAD must not enter the PR contribution range. + commit(repo, "GitHub temporary test merge") + title = "ci: $(touch SENTINEL)" if title_valid else "Update commit policy" + event = repo / "event.json" + event.write_text(json.dumps({"pull_request": {"base": {"sha": base}, "head": {"sha": head}, "title": title}})) + result = subprocess.run( + [sys.executable, "-I", str(SCRIPT), "--repo", str(repo), "--event", str(event)], + capture_output=True, + text=True, + cwd=repo, + ) + assert result.returncode == (0 if title_valid else 1), result.stderr + assert not (repo / "SENTINEL").exists() + if not title_valid: + assert "FAIL PR title" in result.stderr + + +@pytest.mark.parametrize("head", ["main", "missing-object", "--help"]) +def test_uninspectable_or_empty_range_fails_closed(history, head): + repo, base = history + result = check(repo, base, head=head) + assert result.returncode == 2 + assert "could not inspect the range" in result.stderr