-
Notifications
You must be signed in to change notification settings - Fork 91
fix(#6688): add per-commit bot email detection for DCO classification #6780
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
b0dd2b1
9294369
32a57dc
5e4c8d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,3 +19,43 @@ When referencing bot identities in code (e.g., trusted actor lists, dispatch fil | |
| **REST vs. GraphQL login format:** the `[bot]` suffix above is the REST/App-slug form. GitHub's GraphQL API omits it — a bot author's `login` field comes back as `fullsend-ai-coder`, not `fullsend-ai-coder[bot]`, with `__typename: "Bot"`. Comparing a GraphQL-sourced login against a literal `"...[bot]"` string never matches (see #5575) — match on `__typename == "Bot"` plus the un-suffixed login instead. | ||
|
|
||
| **`gh pr view --json` format:** the `gh pr view --json author` CLI command uses a different schema than raw GraphQL — it exposes `.author.is_bot` (boolean) and `.author.login` (with an `app/` prefix, e.g. `app/fullsend-ai-coder`), but does **not** expose `__typename`. When using `gh pr view --json`, check `.author.is_bot == true` plus `.author.login` against the `app/`-prefixed name (see #5536). | ||
|
|
||
| ## Per-commit DCO classification | ||
|
|
||
| DCO (Developer Certificate of Origin) eligibility must be classified **per commit**, using the commit author/committer email — never by the fact that a bot-triggered run is operating on the branch. Mixed-author branches (human commits + bot commits) are common on fix-agent PRs. | ||
|
|
||
| **Rules:** | ||
|
|
||
| 1. **Bot-authored commits** (committer email matches `<digits>+<slug>[bot]@users.noreply.github.com`) are exempt from DCO. They must **not** carry a `Signed-off-by` trailer. The Probot DCO app auto-skips them. | ||
| 2. **Human-authored commits** require valid `Signed-off-by` trailers. These trailers must be **preserved** through any rebase, amend, or history rewrite performed by post-scripts or validation logic. | ||
| 3. **Never use branch-wide `git filter-branch --msg-filter`** to strip `Signed-off-by` trailers. This destroys valid human attestations on mixed-author branches. See #6688 for the incident this caused. | ||
|
|
||
| **Identifying bot commits in Go code:** | ||
|
|
||
| ```go | ||
| import "github.com/fullsend-ai/fullsend/internal/forge" | ||
|
|
||
| if forge.IsBotCommitEmail(committerEmail) { | ||
| // Bot commit — exempt from DCO, must not have Signed-off-by | ||
| } | ||
| ``` | ||
|
|
||
| **Identifying bot commits in shell (post-scripts):** | ||
|
|
||
| ```bash | ||
| # Use GIT_BOT_EMAIL (set by the "Resolve bot identity" workflow step) | ||
| # for exact match, or the regex pattern for general detection. | ||
| committer_email=$(git log -1 --format='%ce' "$sha") | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] internal consistency The shell regex pattern uses Suggested fix: Change |
||
| # Exact match against the resolved bot identity: | ||
| if [[ "$committer_email" == "${GIT_BOT_EMAIL}" ]]; then | ||
| # Bot commit | ||
| fi | ||
|
|
||
| # Pattern match for any GitHub App bot: | ||
| if [[ "$committer_email" =~ ^[0-9]+\+[^@]+\[bot\]@users\.noreply\.github\.com$ ]]; then | ||
| # Bot commit | ||
| fi | ||
| ``` | ||
|
|
||
| **Post-script guidance:** When a post-script needs to validate or modify DCO trailers, it must iterate over commits individually and classify each by its committer email. Only bot-authored commits should be inspected or modified. Human-authored commits must pass through unmodified. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ import ( | |
| "errors" | ||
| "fmt" | ||
| "io" | ||
| "regexp" | ||
| "strings" | ||
| ) | ||
|
|
||
|
|
@@ -378,6 +379,43 @@ func FormatSignOffTrailer(name, email string) (string, error) { | |
| return fmt.Sprintf("Signed-off-by: %s <%s>", name, email), nil | ||
| } | ||
|
|
||
| // botNoreplyRe matches GitHub App bot noreply emails: | ||
| // <digits>+<slug>[bot]@users.noreply.github.com | ||
| // | ||
| // GitHub generates this address from the App's database ID and slug when | ||
| // the App authenticates via an installation token. The Probot DCO app | ||
| // auto-skips commits from authors whose email matches this pattern | ||
| // (user.type == "Bot"), so bot-authored commits must NOT carry a | ||
| // Signed-off-by trailer and human-authored commits on the same branch | ||
| // must NOT have their trailers stripped. | ||
| // | ||
| // See docs/contributing/bot-identities.md for the authoritative identity | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] trust-boundary IsBotCommitEmail classifies commits based solely on regex matching of the committer email, which is user-controlled. Low severity because: (1) the function has no callers yet, (2) the doc comment explicitly warns this is a CI-internal heuristic and must not be the sole enforcement gate, (3) the Probot DCO app and GitHub branch protection provide independent enforcement. |
||
| // table and DCO classification guidance. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] trust boundary - spoofable input IsBotCommitEmail classifies commits as bot-authored based solely on regex matching of the committer email, which is user-controlled. A contributor could set their git committer email to match the bot noreply pattern and bypass DCO classification. Low severity because the function has no callers yet, the Probot DCO app provides independent enforcement, and GitHub branch protection enforces status checks. Suggested fix: Add a doc comment caveat noting this is a CI-internal heuristic and must not be the sole DCO enforcement gate. |
||
| var botNoreplyRe = regexp.MustCompile( | ||
| `^\d+\+[^@]+\[bot\]@users\.noreply\.github\.com$`, | ||
| ) | ||
|
|
||
| // IsBotCommitEmail reports whether email matches the GitHub App bot | ||
| // noreply pattern (<id>+<slug>[bot]@users.noreply.github.com). | ||
| // | ||
| // Use this to classify commits by author type for per-commit DCO | ||
| // decisions: bot-authored commits are exempt from DCO sign-off and | ||
| // must not carry a Signed-off-by trailer; human-authored commits | ||
| // require sign-off and their trailers must be preserved. | ||
| // | ||
| // This is a CI-internal heuristic based on email pattern matching. | ||
| // Because committer email is user-controlled, this function must not | ||
| // be the sole DCO enforcement gate — the Probot DCO app and GitHub | ||
| // branch protection status checks provide independent enforcement. | ||
| // | ||
| // Post-scripts and validation scripts should use this classification | ||
| // (or the equivalent shell pattern) instead of branch-wide operations | ||
| // like git filter-branch --msg-filter, which destroy valid human | ||
| // trailers on mixed-author branches. See #6688. | ||
| func IsBotCommitEmail(email string) bool { | ||
| return botNoreplyRe.MatchString(email) | ||
| } | ||
|
|
||
| // TreeFile represents a file to be committed via the Git Trees API. | ||
| // Mode controls file permissions: "100644" for regular files, | ||
| // "100755" for executable files (e.g., shell scripts). | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[medium] internal consistency
The shell example uses git log -1 --format=%ae (author email) but assigns the result to a variable named committer_email, and the surrounding prose (Rule 1) says to classify by committer email. The %ae format specifier returns the author email; %ce is committer email. The documentation is internally contradictory and could mislead a future implementer.
Suggested fix: Change %ae to %ce in the shell example to match the documented intent of classifying by committer email, or change the prose to say author email and update the Go function doc comment accordingly.