Skip to content

ci: require a Signed-off-by on every commit in a pull request - #355

Closed
Minipada wants to merge 1 commit into
jazzyfrom
feature/303-add-a-dco-check-to-pull-requests
Closed

ci: require a Signed-off-by on every commit in a pull request#355
Minipada wants to merge 1 commit into
jazzyfrom
feature/303-add-a-dco-check-to-pull-requests

Conversation

@Minipada

@Minipada Minipada commented Aug 17, 2026

Copy link
Copy Markdown
Owner

Closes #303

What I did

The premise of the issue turned out to be half wrong, so this PR adds no new checker.

The repo already had a DCO check. The probot DCO app is installed and posts a DCO check run on every pull request — green on #353, so it predates this work. It already covered three of the four acceptance criteria: it fails an unsigned PR, it names the offending commits, and it only looks at the commits the PR adds. What was missing was everything in the tree — no DCO text, no git commit -s in the contributing guide — plus enforcement, since jazzy had no branch protection and no rulesets at all, so the check reported an unsigned PR and the merge went through anyway.

What shipped:

  • DCO at the repo root — verbatim v1.1 text from developercertificate.org, so the sign-off points at a file in the tree rather than a URL that can change under the project.
  • doc/src/dc/contributing.md — "Signing off your commits": what the trailer certifies, that it has to match user.name/user.email, git commit -s, git commit --amend -s --no-edit, git rebase --signoff origin/jazzy + --force-with-lease, and a one-liner for a pre-push eyeball:
    git log --no-merges --format='%h %s → %(trailers:key=Signed-off-by,valueonly,separator=%x2C%x20)' origin/jazzy..HEAD
    An empty right-hand side is a commit with no sign-off. Matching checklist item in .github/PULL_REQUEST_TEMPLATE.md.
  • Branch protection on jazzyDCO is now its one required status check (pinned to the app), with enforce_admins: false so maintainer pushes to the branch still work, and force-pushes and deletions disabled.

How I did it

Two attempts at an in-repo checker were written and then deleted along the way (the branch is squashed to one commit, so they are recorded in progress.txt rather than in history):

  1. .github/workflows/dco.yaml running a script over base.sha..head.sha. It went green on real CI (job "Sign-off") and was removed once the app was found — two identical gates on every PR is noise, and it was the weaker of the two: it only required a well-formed trailer to be present, where the app requires it to match the commit author.
  2. tools/ci/dco_check.sh, kept briefly as the local half. Also removed: with no job running it, it was a hand-reimplementation of a rule it could never be verified against, and its failure mode is the bad one — drift toward leniency means it passes locally while the app fails, which is worse than no local check. A documented one-liner does the same job with nothing to maintain.

Two traps worth recording (both are in progress.txt, since any future in-repo git-message check walks into them):

  • git show -s --format=%B "$sha" | grep -Eiq … is wrong under set -o pipefail. grep -q exits at the first match, git takes SIGPIPE, the pipeline reports 141, and a signed commit reads as unsigned — intermittently, and only for long messages.
  • while read … < <(git rev-list "$RANGE") swallows an unresolvable range and then reports "every commit is signed off" over a list it never read.

How I tested

The deleted script was tested against throwaway repos with core.hooksPath pointed at nothing, so the sign-off wasn't added behind the test's back — this machine has a global prepare-commit-msg hook that signs every commit, which silently made the first two attempts at that test pass for the wrong reason. Missing trailer, sign-off by a different identity, lowercase signed-off-by: with a case-differing address, a trailer under a body and a Co-Authored-By: line, author ≠ committer signed by the committer, two trailers where one matches, an unsigned merge commit and a nonexistent range all behaved correctly before it was removed.

The one-liner that replaced it was run against the same repos: unsigned, wrong-identity and correctly-signed commits are distinguishable at a glance. On the previous head every check was green — required DCO, Format, build, build-workspace, build-e2e-image, e2e, sim, codecov — and prek run --all-files --skip build-doc passes.

Types of changes

  • Other (repo policy: DCO text, docs, branch protection)

I'm not a dummy, so I've checked these

  • 📑 I documented correctly following our guidelines
  • 💯 I tested locally and it is working
  • 🟢 My code does not fail neither code linting checks nor unit test.
  • ✍️ Every commit is signed off (git commit -s)

🤖 Generated with Claude Code

https://claude.ai/code/session_01PUmx6UnU9fuPLg5q385pG2

@codecov

codecov Bot commented Aug 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.54%. Comparing base (1f77002) to head (48f983b).

Additional details and impacted files
@@           Coverage Diff           @@
##            jazzy     #355   +/-   ##
=======================================
  Coverage   67.54%   67.54%           
=======================================
  Files          95       95           
  Lines        5884     5884           
=======================================
  Hits         3974     3974           
  Misses       1910     1910           
Flag Coverage Δ
cpp-jazzy 67.54% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

The repo already had a sign-off check -- the probot DCO app posts one on every pull
request and has all along -- but nothing in the tree said so, nothing explained how to
comply, and `jazzy` had no branch protection or rulesets at all, so the check reported an
unsigned PR and the merge went through regardless. This adds the parts that were actually
missing rather than a second checker.

`DCO` at the repo root is the verbatim v1.1 text from developercertificate.org, so the
sign-off points at a file in the tree rather than a URL that can change under the project.
There is no agreement to sign and no signature records to keep -- the trailer is the
certification, and it is the convention ROS contributors already expect, which is why a
DCO and not a CLA.

The contributing guide gains a section on what the trailer certifies, that it has to match
user.name/user.email, `git commit -s`, amending, `git rebase --signoff`, and a `git log
--format='%h %s -> %(trailers:key=Signed-off-by,valueonly)'` one-liner for a pre-push
eyeball, where an empty right-hand side is a commit with no sign-off. The PR template's
checklist gains a matching item.

Enforcement was the other half: `jazzy` now requires `DCO` as its one status check, pinned
to the app, with enforce_admins off so maintainer pushes to the branch still work, and
force-pushes and deletions disabled.

progress.txt records the two in-repo checkers written and deleted on the way here (a
workflow, then a local script) and why, plus the two pipefail/process-substitution traps
any future git-message check in this repo will walk into.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PUmx6UnU9fuPLg5q385pG2
Signed-off-by: David Bensoussan <d.bensoussan@proton.me>
@Minipada
Minipada force-pushed the feature/303-add-a-dco-check-to-pull-requests branch from 8466282 to 48f983b Compare August 17, 2026 22:21
@Minipada Minipada mentioned this pull request Aug 17, 2026
4 tasks
@Minipada

Copy link
Copy Markdown
Owner Author

Closing unmerged — the repo already had the DCO check this PR set out to add.

The probot DCO app is installed and posts the required DCO check on every pull request, so the workflow and the local script written here were both redundant and were deleted during review. What remained was a DCO text file and documentation, judged not worth the footprint.

The one thing that was missing — enforcement — is already done and does not depend on this PR: jazzy now requires the DCO check, with enforce_admins: false and force-pushes/deletions disabled. Closing this changes none of that.

See #303 for the full record. The branch is kept, so the history (workflow, local check, and the reasons each went) is recoverable if it is ever wanted.

@Minipada Minipada closed this Aug 17, 2026
@Minipada
Minipada deleted the feature/303-add-a-dco-check-to-pull-requests branch September 2, 2026 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant