Add opt-in GPG-signed commits via GitHub API by dragid10 · Pull Request #53 · sanitizers/patchback-github-app

dragid10 · 2026-04-15T17:22:40Z

Summary

Backport commits are now created through GitHub's Git Data REST API, so they are automatically signed by GitHub's web-flow GPG key and show as "Verified". No GPG key management needed — no new permissions required.

Git subprocess operations and GitHub API calls have been extracted into dedicated modules, following the existing one-module-per-API pattern (checks_api.py, comments_api.py, locking_api.py):

git_cli.py — git subprocess operations (clone, fetch, cherry-pick, temp ref upload)
git_api.py — GitHub Git Data REST API (GitAPI class with get_branch_head_sha, create_commit, create_branch)
event_handlers.py — simplified to orchestration; git and API logic moved out

How it works

Cherry-pick runs locally as before (conflict detection, rename handling, --mainline for merge commits)
Tree SHA and commit message are read from the local result
Objects are uploaded to GitHub via a temporary ref (prefixed with the backport branch name for ruleset compatibility, with a random suffix)
Temp ref is deleted
A signed commit is created via POST /repos/{owner}/{repo}/git/commits
The backport branch is created via POST /repos/{owner}/{repo}/git/refs

Ref: #1

Test results

Tested end-to-end with a real GitHub App on dragid10/patchback-test-repo:

PR #10 — commit d2cbe04: committer GitHub, verified: true

Test plan

End-to-end test with real GitHub App — signed commit shows as Verified
Unit tests for GitAPI methods and error handling (23 tests, all passing)
Verified all try blocks have a single instruction
Verified no unused imports or dead code (pyflakes, vulture)

Note: The pre-commit.ci failure is a pre-existing issue (no .pre-commit-config.yaml in the repo) — same failure on PR #50.

webknjaz · 2026-04-15T17:58:57Z

-) -> None:
-    """Returns a branch with backported PR pushed to GitHub.
+        *, signed_commits: bool = False,
+) -> tuple:


This is a bad type. It's full of typing.Any and contradicts the docstring claim.

updated to tuple[str, str] | None

Still a bad API. I didn't get to record all the feedback earlier. If you want to return a bunch of related things, have an object for that. A tuple with arbitrary strings isn't really usable. Although, I'm yet to check the other places for why you thought it's needed.

webknjaz · 2026-04-15T18:35:24Z

Adds a signed_commits setting (default false) to the per-repo .github/patchback.yml config

What's the justification for having this optional? Why do we need a feature toggle?

webknjaz · 2026-04-15T18:44:25Z

+        # Create a signed commit via the GitHub API and point a new
+        # branch at it. Commits created through the Git Data API are
+        # automatically signed by GitHub's web-flow GPG key and show
+        # as "Verified".


Not sure if this bunch of logic probably belongs on this abstraction layer.

webknjaz · 2026-04-15T18:46:54Z

+        # branch at it. Commits created through the Git Data API are
+        # automatically signed by GitHub's web-flow GPG key and show
+        # as "Verified".
+        tree_sha, commit_message = sync_result


An API with hopefully-in-sync-number-of-tuple-items isn't exactly nice to work with. This should be an object. We probably need to move Git REST API interaction into a dedicated class, just like with the comments, the statuses and similar things.

dragid10 · 2026-04-15T18:48:46Z

Adds a signed_commits setting (default false) to the per-repo .github/patchback.yml config

What's the justification for having this optional? Why do we need a feature toggle?

I imagine most wouldn't care if this was the default setting. but since going through the GitHub web signing changes what the committer actually looks like (changes to GitHub), if they were not aware of this change, they might think it odd.

And I've also learned over the years that some people just want the choice regardless of if they exercise it or not 😅
Not particularly strong justification, but it felt nicer to treat it as opt-in

webknjaz · 2026-04-15T18:49:03Z

+        # automatically signed by GitHub's web-flow GPG key and show
+        # as "Verified".
+        tree_sha, commit_message = sync_result
+        try:


try-blocks must have one single instruction. Otherwise, this causes multiple poorly-visible error handling branches that can originate on multiple lines that are difficult to guess just by looking at them.

webknjaz · 2026-04-15T18:53:05Z

+                pr_number, target_branch,
+            )
+            await pr_reporter.finish_reporting(
+                subtitle='💔 cherry-picking failed — could not push',


push? this error message is identical to another place, making it difficult to distinguish between them in the logs and the comments. We should make sure to include the context unique to each situation.

webknjaz · 2026-04-15T18:56:15Z

+                logger.info('Git objects uploaded successfully')
+            finally:
+                # Always clean up the temporary ref
+                try:


Avoid nested try-in-finally. This is difficult to reason about. Additionally, such cases could be abstracted away by a CM.

webknjaz · 2026-04-15T18:58:00Z

+            else:
+                logger.info('Git objects uploaded successfully')
+            finally:
+                # Always clean up the temporary ref


Code should speak for itself. If you feel like the code comment is needed, most of the time it's a sign of abstraction layer leaks or problematic variable naming.

webknjaz · 2026-04-15T19:00:59Z

+            # trees) to GitHub. The ref is deleted immediately after —
+            # we only need the objects to exist on GitHub so the API
+            # commit can reference the tree SHA.
+            temp_ref = f'refs/patchback-tmp/{secrets.token_hex(16)}'


This should probably be at least prefixed the same way as the final branch name. Projects with branch protection rulesets might allow certain patterns but not others. Perhaps, just use the final branch name and append /{a_random_thing} at the end — this will likely avoid most of such cases.

webknjaz · 2026-04-15T19:01:56Z

+                raise PermissionError(
+                    'Current GitHub App installation does not grant '
+                    'sufficient privileges for pushing to '
+                    f'{repo_remote}. Lacking '


This should mention the ref name.

webknjaz · 2026-04-15T19:02:20Z

+                    'sufficient privileges for pushing to '
+                    f'{repo_remote}. Lacking '
+                    '`Contents: write` or `Workflows: write` '
+                    'permissions are known to cause this.\n\n'


We may also want to hint the users to check their rulesets for signs of blocking the pushes.

webknjaz · 2026-04-15T19:03:25Z

+                # Always clean up the temporary ref
+                try:
+                    spawn_proc(
+                        *git_cmd, 'push', 'origin', f':{temp_ref}',


That's a retro-style command. Today, it's git push -d ...

webknjaz · 2026-04-15T19:07:09Z

+    objects via a temporary ref for signed commit creation.
+
+    When ``signed_commits`` is enabled, returns a
+    ``(tree_sha, commit_message)`` tuple. Otherwise returns ``None``.


This is probably not needed in the first place. We shouldn't have parts of commit manipulation in disconnected places, anyway.

webknjaz · 2026-04-15T19:09:08Z

Adds a signed_commits setting (default false) to the per-repo .github/patchback.yml config

What's the justification for having this optional? Why do we need a feature toggle?

I imagine most wouldn't care if this was the default setting. but since going through the GitHub web signing changes what the committer actually looks like (changes to GitHub), if they were not aware of this change, they might think it odd.

And I've also learned over the years that some people just want the choice regardless of if they exercise it or not 😅 Not particularly strong justification, but it felt nicer to treat it as opt-in

Let's just get rid of this until somebody asks. I don't want additional complexity that potentially nobody even wants in the first place that I'd be left with to maintain. Less is more.

Also, the committer thing is fine — the author is still the GH App which is what's most important: https://api.github.com/repos/dragid10/patchback-test-repo/commits/9b66d01f772a1fd2ba1c010524c5f24872da1895

webknjaz · 2026-04-15T19:14:36Z

How it works

1. Cherry-pick runs locally as before (conflict detection, rename handling, `--mainline` for merge commits)

2. Tree SHA and commit message are read from the local result

3. Objects are uploaded to GitHub via a temporary ref (deleted immediately after)

4. A signed commit is created via `POST /repos/{owner}/{repo}/git/commits`

5. The backport branch is created via `POST /repos/{owner}/{repo}/git/refs`

I'm wondering if it'd be a good idea to provide this as a part of the utils in octomachinery. But this probably goes into the same bucket as sanitizers/octomachinery#4..

Still, it'd be good to have a well-isolated relocatable piece of logic in a helper for better structuring. The amount of logic in the event_handlers is growing and is rather unhelpful. With the state of the project where we don't have a CI or tests, not even the linters or the workflow tooling, I wouldn't like to keep the cognitive complexity in check.

Note: it'd be good to have a helper capable of going through a chain of commits and signing each of those one by one. This is a little out of the scope but keep it in mind when working on other stuff.

webknjaz · 2026-04-15T19:17:50Z

I'm done w/ the review round for now. Since you're already looking into this, I'm open to getting a CLAUDE.md with the context in a standalone PR.

Create backport commits through GitHub's Git Data REST API so they are automatically signed by GitHub's web-flow GPG key and show as "Verified". The cherry-pick still runs locally via git subprocess. Extract git subprocess operations into git_cli.py and GitHub API calls into git_api.py, following the existing one-module-per-API pattern (checks_api.py, comments_api.py, locking_api.py). Ref: sanitizers#1 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

dragid10 · 2026-04-21T13:29:24Z

@webknjaz I pushed some new commits to address your feedback!

webknjaz · 2026-04-21T14:43:44Z

Yeah, I liked some parts of the previous state more. Now, I'll have to find enough time to compose a full review. No ETA for that. FWIW, it's best to have smaller PRs as atomic patches are easier to review and merge-in faster + “no-go” things that may happen to be in the same PR wouldn't be blocking. One huge commit is going to take time to polish.

webknjaz · 2026-04-21T14:51:24Z

+            bad_req_err.status_code != http.client.FORBIDDEN or
+            str(bad_req_err) != 'Resource not accessible by integration'
+    ):
+        raise


Does this actually work? What if it's called outside of the exception handling context? Seems rather fragile.

dragid10 · 2026-04-21T15:04:39Z

Yeah, I liked some parts of the previous state more. Now, I'll have to find enough time to compose a full review. No ETA for that. FWIW, it's best to have smaller PRs as atomic patches are easier to review and merge-in faster + “no-go” things that may happen to be in the same PR wouldn't be blocking. One huge commit is going to take time to polish.

okay would you prefer I go with this direction now?

webknjaz · 2026-04-21T17:25:04Z

I think it's a good idea to identify the smallest improvement you can distill and work on that first. In a standalone PR. We may need to pre-agree on what that would be. This could be moving some helpers into a separate model. This could be agreeing to push the low-level sync operations closer to the subprocess invocations and making more logic async-native.

webknjaz · 2026-04-21T18:14:08Z

FTR, I've also raised the question of having GH App-bound signing keys that would not require the API dance with some GH folks in private spaces. So long-term, this can end up being simplified if they take on the idea I presented. But probably not in months.

webknjaz · 2026-04-22T14:48:35Z

I think it's a good idea to identify the smallest improvement you can distill and work on that first. In a standalone PR. We may need to pre-agree on what that would be. This could be moving some helpers into a separate model. This could be agreeing to push the low-level sync operations closer to the subprocess invocations and making more logic async-native.

Apparently, I forgot to click "Submit" yesterday, and this has been sitting unsent in my browser:
A good place to start could be separating refactoring and functional changes.

webknjaz reviewed Apr 15, 2026

View reviewed changes

webknjaz linked an issue Apr 15, 2026 that may be closed by this pull request

[research] Producing GPG-signed commits #1

Open

webknjaz reviewed Apr 15, 2026

View reviewed changes

webknjaz added the enhancement New feature or request label Apr 15, 2026

dragid10 force-pushed the feat/gpg-signed-commits branch from 4066988 to b98f3ba Compare April 16, 2026 03:18

dragid10 requested a review from webknjaz April 16, 2026 14:23

webknjaz reviewed Apr 21, 2026

View reviewed changes

Uh oh!

Conversation

dragid10 commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

How it works

Test results

Test plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webknjaz commented Apr 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dragid10 commented Apr 15, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webknjaz commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

webknjaz commented Apr 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How it works

Uh oh!

webknjaz commented Apr 15, 2026

Uh oh!

dragid10 commented Apr 21, 2026

Uh oh!

webknjaz commented Apr 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dragid10 commented Apr 21, 2026

Uh oh!

webknjaz commented Apr 21, 2026

Uh oh!

webknjaz commented Apr 21, 2026

Uh oh!

webknjaz commented Apr 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dragid10 commented Apr 15, 2026 •

edited

Loading

webknjaz commented Apr 15, 2026 •

edited

Loading

webknjaz commented Apr 15, 2026 •

edited

Loading