fix: restore original author on cherry-pick for DCO compliance#1073
Open
fix: restore original author on cherry-pick for DCO compliance#1073
Conversation
GitHub squash-merges rewrite the commit author email to the noreply format (e.g., 86722603+user@users.noreply.github.com), but preserve the original email in the Signed-off-by trailer. When git cherry-pick replays such a commit, the email mismatch causes DCO checks to fail. Add _restore_original_author_for_cherry_pick() that fetches the original PR commits, finds the Signed-off-by trailer, and amends the cherry-picked commit author to match. Only amends when the original PR had a sign-off. Called after successful cherry-pick (clean or AI-resolved) before push. Closes: #1072 Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: rnetser <rnetser@redhat.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds methods to locate a PR's Changes
Sequence DiagramsequenceDiagram
participant WH as Webhook Handler
participant RH as RunnerHandler
participant GHA as GitHub API
participant GIT as Git CLI
WH->>RH: cherry_pick(pull_request, target_branch, ...)
RH->>GIT: git cherry-pick <commit>
alt conflict
RH->>RH: resolve conflicts (AI/manual)
RH->>GIT: git cherry-pick --continue
end
RH->>GHA: GET /repos/.../pulls/<orig>/commits
GHA-->>RH: commits[]
RH->>RH: scan commits newest→oldest for Signed-off-by
RH->>GIT: git log -1 --format=%an%n%ae
GIT-->>RH: current_author_name/email
alt sign-off found AND emails differ
RH->>GIT: git log -1 --format=%B
GIT-->>RH: commit_message
RH->>GIT: git commit --amend --author="Name <email>" [-m / -F / --no-edit]
GIT-->>RH: amend result (success/fail)
else no sign-off or emails match
RH->>RH: skip amend
end
RH->>GIT: git push origin <branch>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Collaborator
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webhook_server/libs/handlers/runner_handler.py`:
- Around line 785-808: The code logs and embeds the full real author identity
when amending commits (see run_command usage building f"{git_cmd} commit --amend
--no-edit --author={shlex.quote(f'{author_name} <{author_email}>')}" and the
subsequent logger.info), which risks leaking personal emails; change this by (1)
adding author_email (and optionally author_name) to the run_command
redact_secrets list so command traces do not contain the raw identity, (2) avoid
emitting the full "Name <email>" in logger.info/logger.warning — instead log a
redacted form (e.g., mask the local-part of the email or only log the domain) or
simply log "Restored original author" without the literal, and (3) optionally
build an author_cmd variable for the commit string and a separate
redacted_author string for logs to keep the commit and logs consistent
(referencing the commit amend call and logger.info in runner_handler.py).
- Around line 760-809: Wrap the external PyGithub calls (the call to
pull_request.get_commits() via github_api_call and the per-commit access to
c.commit.message) in try/except that catches Exception, immediately re-raises
asyncio.CancelledError, and for other exceptions uses self.logger.exception(...)
to log the full traceback and then return False so the cherry-pick flow
continues (best-effort author restore) instead of propagating the error; update
the code around commits = await github_api_call(...) and the loop that reads
commit_msg (referencing pull_request.get_commits(), c.commit.message,
github_api_call, and cherry_pick()/the surrounding method) to implement this
behavior.
In `@webhook_server/tests/test_runner_handler.py`:
- Around line 1038-1041: The test stubs out
_restore_original_author_for_cherry_pick but never asserts it was awaited;
update the patch to assign the AsyncMock to a local variable (e.g., mock_restore
= AsyncMock(return_value=False) and patch.object(runner_handler,
"_restore_original_author_for_cherry_pick", new=mock_restore)), call the code
path (await runner_handler.cherry_pick(...)), then assert the hook was actually
awaited with mock_restore.assert_awaited_once() or
assert_awaited_with(expected_args) to prove cherry_pick awaited the restore
hook.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 035a90c9-e333-45e7-b297-7abf8bec4ecb
📒 Files selected for processing (2)
webhook_server/libs/handlers/runner_handler.pywebhook_server/tests/test_runner_handler.py
…r, assert hook - Wrap PyGithub calls in try/except so author restore is best-effort and does not abort cherry-pick on API failure - Redact author name/email from run_command logs and simplify INFO message - Assert _restore_original_author_for_cherry_pick is awaited in cherry-pick success test Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: rnetser <rnetser@redhat.com>
Collaborator
Author
|
/build-and-push-container |
Collaborator
|
New container for ghcr.io/myk-org/github-webhook-server:pr-1073 published |
Collaborator
Author
|
/verified |
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webhook_server/libs/handlers/runner_handler.py`:
- Around line 804-811: The warning log at self.logger.warning uses the raw
stderr returned from run_command (err) which can contain sensitive author info;
before logging, redact/mask secrets (github_token, author_spec, author_email,
author_name) from err using the same redaction/masking logic used by run_command
(or a shared redact utility) and then log the redacted message (e.g., compute
redacted_err = redact(err, [github_token, author_spec, author_email,
author_name]) and use that in the f"{self.log_prefix} Failed to amend..." call);
update the code paths around run_command, self.logger.warning, and any helper
used for redaction to ensure stderr is never logged unredacted.
In `@webhook_server/tests/test_runner_handler.py`:
- Around line 1912-2185: Add two tests for
RunnerHandler._restore_original_author_for_cherry_pick that cover
github_api_call failure paths: (1) patch
"webhook_server.libs.handlers.runner_handler.github_api_call" to raise a regular
Exception when calling pull_request.get_commits (use the existing runner_handler
and _make_pr_with_commits helpers), assert the method returns False and that
runner_handler.github_webhook.logger.exception was called; (2) patch
github_api_call to raise asyncio.CancelledError and assert the CancelledError is
propagated (re-raised). Ensure both tests target the same function name
_restore_original_author_for_cherry_pick and verify behavior per
logger.exception and return False for the non-cancelled error path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1cfb4e4d-7a01-450d-882d-84be721a11d7
📒 Files selected for processing (2)
webhook_server/libs/handlers/runner_handler.pywebhook_server/tests/test_runner_handler.py
- Redact author identity from stderr before logging on amend failure - Add test for github_api_call exception → returns False (best-effort) - Add test for asyncio.CancelledError re-raise Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: rnetser <rnetser@redhat.com>
- Move redact_list creation before git log reads so author identity is redacted in %an%ae and %B stdout logging - Switch happy-path tests from mocking github_api_call to mocking asyncio.to_thread, exercising the real github_api_call wrapper Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: rnetser <rnetser@redhat.com>
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webhook_server/libs/handlers/runner_handler.py`:
- Around line 770-830: The code currently finds a source_commit by detecting any
"Signed-off-by:" but then ignores the parsed trailer and uses
source_commit.commit.author for the restored identity and deletes all
Signed-off-by trailers when building amended_msg, which drops other sign-offs;
instead, parse the exact Signed-off-by name and email from commit_msg (the one
used to select source_commit) and use that parsed identity as the restored
author (rather than source_commit.commit.author), then when filtering trailers
only remove trailers that match the parsed Signed-off-by (same name/email) and
preserve all other Signed-off-by lines so multi-signer sign-offs are retained;
update references around commit_msg, source_commit, author_name/author_email,
and the filtering logic that builds amended_msg accordingly.
In `@webhook_server/tests/test_runner_handler.py`:
- Around line 1976-2010: The test test_amend_when_email_mismatch currently only
asserts the --author was passed; update it to also verify the commit message
file used during "git commit --amend" contains the rewritten Signed-off-by
trailer. Modify the run_command_side_effect (used in this test) to detect the
"commit --amend" invocation that includes a "-F" message file, read that file
path from the command string and assert its contents include "Signed-off-by:
Jenia Peimer <jpeimer@redhat.com>" and do not include the old "jpeimer
<jpeimer@redhat.com>" trailer; ensure this assertion targets the same test and
the call flow exercised by
runner_handler._restore_original_author_for_cherry_pick so the test fails if the
message was not rewritten.
- Around line 2148-2186: The test must prove newest→oldest search order: give
both commit1 and commit2 distinct "Signed-off-by" lines (e.g., add
"Signed-off-by: Author One <one@example.com>" to commit1.commit.message while
keeping commit2.commit.message with Author Two), and adjust any mocked git body
responses (run_command_side_effect for "log -1 --format=%B") if needed so the
code sees the sign-offs; keep the existing assertion that the "commit --amend"
command contains "Author Two <two@example.com>" to assert the newer commit's
Signed-off-by wins when _restore_original_author_for_cherry_pick processes
mock_pr.get_commits (commit1, commit2).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c4f4955f-6cd7-4752-8499-d0200d8bc715
📒 Files selected for processing (2)
webhook_server/libs/handlers/runner_handler.pywebhook_server/tests/test_runner_handler.py
- Assert -F message file contains corrected Signed-off-by trailer and does not contain the old one in test_amend_when_email_mismatch - Give both commits sign-offs in test_multiple_commits_finds_signoff_in_last to prove newest-first ordering (assert Author One not in command) Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: rnetser <rnetser@redhat.com>
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Use merge commit (pull_request.merge_commit_sha) as primary source for Signed-off-by and author identity. Falls back to scanning PR commits for regular (non-squash) merges where the merge commit lacks a sign-off. Extract _find_signoff_source() helper for the two-tier lookup. Remove tempfile — use shlex.quote() with -m flag directly. Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: rnetser <rnetser@redhat.com>
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webhook_server/tests/test_runner_handler.py (1)
1588-1616:⚠️ Potential issue | 🟡 MinorMEDIUM: Assert the restore hook runs on AI-resolved cherry-picks too.
These branches patch
_restore_original_author_for_cherry_pick()but never assert it was awaited. If the call regresses to only the clean cherry-pick path, these AI conflict-resolution tests still pass even though DCO restoration would be skipped.🧪 Proposed assertion pattern
with ( - patch.object(runner_handler, "_restore_original_author_for_cherry_pick", new=AsyncMock(return_value=False)), + patch.object( + runner_handler, + "_restore_original_author_for_cherry_pick", + new=AsyncMock(return_value=False), + ) as mock_restore_author, patch.object(runner_handler, "is_branch_exists", new=AsyncMock(return_value=Mock())), ): @@ await runner_handler.cherry_pick(mock_pull_request, "main") + mock_restore_author.assert_awaited_once() mock_set_success.assert_called_once()Also applies to: 1662-1690
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@webhook_server/tests/test_runner_handler.py` around lines 1588 - 1616, The tests patch _restore_original_author_for_cherry_pick but never assert it was awaited, so update the AI-resolved cherry-pick tests to verify the restore hook runs: replace the current patch of runner_handler._restore_original_author_for_cherry_pick with an AsyncMock assigned to a local variable (e.g., mock_restore = AsyncMock(return_value=False)), use patch.object(..., new=mock_restore), then after awaiting runner_handler.cherry_pick(...) assert mock_restore.await_count or mock_restore.assert_awaited() (or assert_called_once() on awaitable) to ensure the DCO restoration path was executed; apply the same change to the corresponding block around lines 1662-1690 as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webhook_server/libs/handlers/runner_handler.py`:
- Around line 761-775: The current regex that looks for Signed-off-by in commit
messages (used when inspecting merge_commit and later in the commits loop via
the signoff_match variable and commit_msg) can match text anywhere in the
message body; update both uses to only match trailer lines by anchoring to line
boundaries and enabling multiline mode (use a regex like ^Signed-off-by: ...
with re.MULTILINE) so only true DCO trailer lines are returned; make the same
change for the merge_commit block and the for-commit loop that uses
github_api_call and signoff_match.
- Around line 818-856: The early return in
_restore_original_author_for_cherry_pick (when current author name/email match
author_name and author_email) prevents the Signed-off-by trailer normalization;
remove the return False and instead allow the code to continue to the
commit-message read/amend path so the method always rewrites trailers to a
single synthesized "Signed-off-by: {author_name} <{author_email}>" (you can
still skip changing author metadata if desired but must not skip building
amended_msg); specifically, edit the branch that checks current_author_info (the
block referencing current_author_info, author_name, author_email and the logged
"Author already matches" message) to not return early and ensure amended_msg is
constructed and used later when computing msg_flag (which references
amended_msg) so trailers are normalized even when the author already matches.
In `@webhook_server/tests/test_runner_handler.py`:
- Around line 1972-1975: Tests currently only set commit.author.name on
mock_merge_commit and similar mocks; add commit.author.email to those mocks so
the tests pin behavior to the git author email (not the Signed-off-by trailer).
Update the mocks used around mock_merge_commit (and the other instance at the
2116-2120 area) to set mock_merge_commit.commit.author.email = <expected_email>,
then add a case where the trailer email differs from that author email and
assert that _restore_original_author_for_cherry_pick() (or the code path that
uses source_commit.commit.author) results in the amended author/trailer using
commit.author.email.
---
Outside diff comments:
In `@webhook_server/tests/test_runner_handler.py`:
- Around line 1588-1616: The tests patch
_restore_original_author_for_cherry_pick but never assert it was awaited, so
update the AI-resolved cherry-pick tests to verify the restore hook runs:
replace the current patch of
runner_handler._restore_original_author_for_cherry_pick with an AsyncMock
assigned to a local variable (e.g., mock_restore =
AsyncMock(return_value=False)), use patch.object(..., new=mock_restore), then
after awaiting runner_handler.cherry_pick(...) assert mock_restore.await_count
or mock_restore.assert_awaited() (or assert_called_once() on awaitable) to
ensure the DCO restoration path was executed; apply the same change to the
corresponding block around lines 1662-1690 as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b69bca14-dde2-40f0-90d6-856e23d78c76
📒 Files selected for processing (2)
webhook_server/libs/handlers/runner_handler.pywebhook_server/tests/test_runner_handler.py
Comment on lines
+761
to
+775
| signoff_match = re.findall(r"Signed-off-by:\s*(.+?)\s*<([^>]+)>", commit_msg) | ||
| if signoff_match: | ||
| return merge_commit, signoff_match[-1][1] | ||
|
|
||
| # Fall back to PR commits (regular merge case) | ||
| commits = await github_api_call( | ||
| lambda: list(pull_request.get_commits()), logger=self.logger, log_prefix=self.log_prefix | ||
| ) | ||
| for commit in reversed(commits): | ||
| commit_msg = await github_api_call( | ||
| lambda c=commit: c.commit.message, logger=self.logger, log_prefix=self.log_prefix | ||
| ) | ||
| signoff_match = re.findall(r"Signed-off-by:\s*(.+?)\s*<([^>]+)>", commit_msg) | ||
| if signoff_match: | ||
| return commit, signoff_match[-1][1] |
There was a problem hiding this comment.
MEDIUM: Match actual Signed-off-by trailer lines only.
The current regex matches Signed-off-by: anywhere in the message body, so explanatory text can be misclassified as a DCO trailer and trigger an incorrect amend. Anchor it to full lines.
Suggested fix
- signoff_match = re.findall(r"Signed-off-by:\s*(.+?)\s*<([^>]+)>", commit_msg)
+ signoff_match = re.findall(r"(?im)^Signed-off-by:\s*(.+?)\s*<([^>\n]+)>\s*$", commit_msg)
@@
- signoff_match = re.findall(r"Signed-off-by:\s*(.+?)\s*<([^>]+)>", commit_msg)
+ signoff_match = re.findall(r"(?im)^Signed-off-by:\s*(.+?)\s*<([^>\n]+)>\s*$", commit_msg)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| signoff_match = re.findall(r"Signed-off-by:\s*(.+?)\s*<([^>]+)>", commit_msg) | |
| if signoff_match: | |
| return merge_commit, signoff_match[-1][1] | |
| # Fall back to PR commits (regular merge case) | |
| commits = await github_api_call( | |
| lambda: list(pull_request.get_commits()), logger=self.logger, log_prefix=self.log_prefix | |
| ) | |
| for commit in reversed(commits): | |
| commit_msg = await github_api_call( | |
| lambda c=commit: c.commit.message, logger=self.logger, log_prefix=self.log_prefix | |
| ) | |
| signoff_match = re.findall(r"Signed-off-by:\s*(.+?)\s*<([^>]+)>", commit_msg) | |
| if signoff_match: | |
| return commit, signoff_match[-1][1] | |
| signoff_match = re.findall(r"(?im)^Signed-off-by:\s*(.+?)\s*<([^>\n]+)>\s*$", commit_msg) | |
| if signoff_match: | |
| return merge_commit, signoff_match[-1][1] | |
| # Fall back to PR commits (regular merge case) | |
| commits = await github_api_call( | |
| lambda: list(pull_request.get_commits()), logger=self.logger, log_prefix=self.log_prefix | |
| ) | |
| for commit in reversed(commits): | |
| commit_msg = await github_api_call( | |
| lambda c=commit: c.commit.message, logger=self.logger, log_prefix=self.log_prefix | |
| ) | |
| signoff_match = re.findall(r"(?im)^Signed-off-by:\s*(.+?)\s*<([^>\n]+)>\s*$", commit_msg) | |
| if signoff_match: | |
| return commit, signoff_match[-1][1] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webhook_server/libs/handlers/runner_handler.py` around lines 761 - 775, The
current regex that looks for Signed-off-by in commit messages (used when
inspecting merge_commit and later in the commits loop via the signoff_match
variable and commit_msg) can match text anywhere in the message body; update
both uses to only match trailer lines by anchoring to line boundaries and
enabling multiline mode (use a regex like ^Signed-off-by: ... with re.MULTILINE)
so only true DCO trailer lines are returned; make the same change for the
merge_commit block and the for-commit loop that uses github_api_call and
signoff_match.
Comment on lines
+818
to
+856
| # Check if the cherry-picked commit author already matches (both name and email) | ||
| rc, current_author_info, _ = await run_command( | ||
| command=f"{git_cmd} log -1 --format=%an%n%ae", | ||
| log_prefix=self.log_prefix, | ||
| redact_secrets=redact_list, | ||
| mask_sensitive=self.github_webhook.mask_sensitive, | ||
| ) | ||
| if not rc: | ||
| self.logger.warning( | ||
| f"{self.log_prefix} Could not read current author info, proceeding with author amend" | ||
| ) | ||
| else: | ||
| info_lines = current_author_info.strip().splitlines() | ||
| if len(info_lines) == 2 and info_lines[0] == author_name and info_lines[1] == author_email: | ||
| self.logger.debug(f"{self.log_prefix} Author already matches original PR commit, no amend needed") | ||
| return False | ||
|
|
||
| # Read the current commit message to fix Signed-off-by trailers | ||
| rc, current_msg, _ = await run_command( | ||
| command=f"{git_cmd} log -1 --format=%B", | ||
| log_prefix=self.log_prefix, | ||
| redact_secrets=redact_list, | ||
| mask_sensitive=self.github_webhook.mask_sensitive, | ||
| ) | ||
| amended_msg: str | None = None | ||
| if not rc: | ||
| self.logger.warning(f"{self.log_prefix} Could not read commit message, amending author only") | ||
| else: | ||
| # Remove all existing Signed-off-by trailers and add the correct one | ||
| msg_lines = current_msg.rstrip().splitlines() | ||
| filtered_lines = [line for line in msg_lines if not re.match(r"Signed-off-by:\s*", line)] | ||
| while filtered_lines and not filtered_lines[-1].strip(): | ||
| filtered_lines.pop() | ||
| filtered_lines.append("") | ||
| filtered_lines.append(f"Signed-off-by: {author_name} <{author_email}>") | ||
| amended_msg = "\n".join(filtered_lines) + "\n" | ||
|
|
||
| # Amend the commit author and optionally the message | ||
| msg_flag = f"-m {shlex.quote(amended_msg)}" if amended_msg else "--no-edit" |
There was a problem hiding this comment.
HIGH: Don’t return before normalizing the sign-off trailer.
Line 833 skips the message rewrite whenever author name/email already match. That leaves short-name trailers such as Signed-off-by: rnetser <...> untouched, even though the method is meant to synthesize a single trailer from the git author identity for DCO compliance.
Suggested fix
+ needs_author_amend = True
# Check if the cherry-picked commit author already matches (both name and email)
rc, current_author_info, _ = await run_command(
command=f"{git_cmd} log -1 --format=%an%n%ae",
log_prefix=self.log_prefix,
redact_secrets=redact_list,
@@
else:
info_lines = current_author_info.strip().splitlines()
if len(info_lines) == 2 and info_lines[0] == author_name and info_lines[1] == author_email:
- self.logger.debug(f"{self.log_prefix} Author already matches original PR commit, no amend needed")
- return False
+ needs_author_amend = False
# Read the current commit message to fix Signed-off-by trailers
rc, current_msg, _ = await run_command(
command=f"{git_cmd} log -1 --format=%B",
@@
amended_msg: str | None = None
+ needs_message_amend = False
if not rc:
self.logger.warning(f"{self.log_prefix} Could not read commit message, amending author only")
else:
# Remove all existing Signed-off-by trailers and add the correct one
msg_lines = current_msg.rstrip().splitlines()
@@
filtered_lines.append("")
filtered_lines.append(f"Signed-off-by: {author_name} <{author_email}>")
amended_msg = "\n".join(filtered_lines) + "\n"
+ needs_message_amend = amended_msg != current_msg
+
+ if not needs_author_amend and not needs_message_amend:
+ self.logger.debug(f"{self.log_prefix} Author and Signed-off-by already match, no amend needed")
+ return False
# Amend the commit author and optionally the message
- msg_flag = f"-m {shlex.quote(amended_msg)}" if amended_msg else "--no-edit"
+ msg_flag = f"-m {shlex.quote(amended_msg)}" if needs_message_amend and amended_msg else "--no-edit"Based on learnings, _restore_original_author_for_cherry_pick() intentionally removes existing Signed-off-by trailers and replaces them with a single trailer synthesized from the git author identity to guarantee DCO compliance.
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 827-827: Logging statement uses f-string
(G004)
[warning] 832-832: Logging statement uses f-string
(G004)
[warning] 844-844: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webhook_server/libs/handlers/runner_handler.py` around lines 818 - 856, The
early return in _restore_original_author_for_cherry_pick (when current author
name/email match author_name and author_email) prevents the Signed-off-by
trailer normalization; remove the return False and instead allow the code to
continue to the commit-message read/amend path so the method always rewrites
trailers to a single synthesized "Signed-off-by: {author_name} <{author_email}>"
(you can still skip changing author metadata if desired but must not skip
building amended_msg); specifically, edit the branch that checks
current_author_info (the block referencing current_author_info, author_name,
author_email and the logged "Author already matches" message) to not return
early and ensure amended_msg is constructed and used later when computing
msg_flag (which references amended_msg) so trailers are normalized even when the
author already matches.
Comment on lines
+1972
to
+1975
| mock_merge_commit = Mock() | ||
| mock_merge_commit.commit.message = merge_commit_msg | ||
| mock_merge_commit.commit.author.name = author_name | ||
|
|
There was a problem hiding this comment.
HIGH: Set source commit author email in these mocks.
These tests don’t pin the behavior that the restored identity comes from the git author object’s email. A regression to using the parsed Signed-off-by email would still pass because the mocks only define author names.
🧪 Proposed test hardening
def _make_merged_pr(
merge_commit_msg: str,
author_name: str = "Test User",
+ author_email: str = "test@example.com",
merge_commit_sha: str = "abc123",
) -> tuple[Mock, Mock]:
@@
mock_merge_commit = Mock()
mock_merge_commit.commit.message = merge_commit_msg
mock_merge_commit.commit.author.name = author_name
+ mock_merge_commit.commit.author.email = author_email pr_commit = Mock()
pr_commit.commit.message = "feat: add feature\n\nSigned-off-by: Test User <test@example.com>\n"
pr_commit.commit.author.name = "Test User"
+ pr_commit.commit.author.email = "test@example.com"
mock_pr.get_commits.return_value = [pr_commit]Consider adding one case where the trailer email differs from commit.author.email and assert the amended author/trailer uses commit.author.email.
Based on learnings: _restore_original_author_for_cherry_pick() intentionally uses source_commit.commit.author.name/email instead of the parsed Signed-off-by trailer identity.
Also applies to: 2116-2120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@webhook_server/tests/test_runner_handler.py` around lines 1972 - 1975, Tests
currently only set commit.author.name on mock_merge_commit and similar mocks;
add commit.author.email to those mocks so the tests pin behavior to the git
author email (not the Signed-off-by trailer). Update the mocks used around
mock_merge_commit (and the other instance at the 2116-2120 area) to set
mock_merge_commit.commit.author.email = <expected_email>, then add a case where
the trailer email differs from that author email and assert that
_restore_original_author_for_cherry_pick() (or the code path that uses
source_commit.commit.author) results in the amended author/trailer using
commit.author.email.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When the webhook server cherry-picks a squash-merged commit, the DCO check on the cherry-pick PR fails because:
86722603+user@users.noreply.github.com)Signed-off-bytrailer preserves the original email (e.g.,user@redhat.com)git cherry-pickreplays the commit with the mismatched emailsSigned-off-byemail matches the commit author email → failsThis requires manual sign-off approval for every cherry-picked PR.
Solution
Add
_restore_original_author_for_cherry_pick()toRunnerHandlerthat:pull_request.get_commits()Signed-off-bytrailer (last commit first)Testing
TestRestoreOriginalAuthorForCherryPickcovering all pathsReferences
Closes: #1072
Assisted-by: Claude noreply@anthropic.com
Summary by CodeRabbit
Bug Fixes
Tests