From 908b4b0d9a1e618be128613ed5768cd23b29beee Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 16 Apr 2026 08:24:49 +0000 Subject: [PATCH 1/8] fix: restore original author on cherry-pick for DCO compliance 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 Signed-off-by: rnetser --- .../libs/handlers/runner_handler.py | 80 +++++ webhook_server/tests/test_runner_handler.py | 301 +++++++++++++++++- 2 files changed, 376 insertions(+), 5 deletions(-) diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index 721d1d42..5014df3b 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -735,6 +735,79 @@ async def is_branch_exists(self, branch: str) -> bool: return False raise + async def _restore_original_author_for_cherry_pick( + self, + pull_request: PullRequest, + git_cmd: str, + github_token: str, + ) -> bool: + """Amend cherry-picked commit author to match the original PR's Signed-off-by. + + GitHub squash-merges rewrite the author email to the noreply format + (e.g., 86722603+user@users.noreply.github.com). When git cherry-pick + replays such a commit, the DCO check fails because the author email + no longer matches the Signed-off-by trailer. + + This method checks the original PR's commits for a Signed-off-by trailer. + If found, it amends the cherry-picked commit's author to match, restoring + the original PR owner's identity for DCO compliance. + + Only amends if the original PR commits had a Signed-off-by. + + Returns True if the commit was amended, False if no amendment was needed or possible. + """ + # Get the original PR's commits to find the Signed-off-by + commits = await github_api_call( + lambda: list(pull_request.get_commits()), logger=self.logger, log_prefix=self.log_prefix + ) + if not commits: + self.logger.debug(f"{self.log_prefix} No commits found in original PR, skipping author restore") + return False + + # Search original PR commits for a Signed-off-by trailer (check last commit first) + author_name: str | None = None + author_email: str | None = None + 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: + author_name, author_email = signoff_match[-1] + author_name = author_name.strip() + break + + if not author_name or not author_email: + self.logger.debug(f"{self.log_prefix} No Signed-off-by in original PR commits, skipping author restore") + return False + + # Check if the cherry-picked commit author already matches + rc, current_author_email, _ = await run_command( + command=f"{git_cmd} log -1 --format=%ae", + log_prefix=self.log_prefix, + redact_secrets=[github_token], + mask_sensitive=self.github_webhook.mask_sensitive, + ) + if not rc: + self.logger.warning(f"{self.log_prefix} Could not read current author email, proceeding with author amend") + elif current_author_email.strip() == author_email: + self.logger.debug(f"{self.log_prefix} Author email already matches original PR sign-off, no amend needed") + return False + + # Amend the commit author to match the original PR's Signed-off-by + rc, _, err = await run_command( + command=f"{git_cmd} commit --amend --no-edit --author={shlex.quote(f'{author_name} <{author_email}>')}", + log_prefix=self.log_prefix, + redact_secrets=[github_token], + mask_sensitive=self.github_webhook.mask_sensitive, + ) + if not rc: + self.logger.warning(f"{self.log_prefix} Failed to amend cherry-pick author for DCO compliance: {err}") + return False + + self.logger.info(f"{self.log_prefix} Restored original author '{author_name} <{author_email}>' on cherry-pick") + return True + async def _resolve_cherry_pick_with_ai( self, worktree_path: str, @@ -1025,6 +1098,13 @@ async def cherry_pick( return cherry_pick_had_conflicts = True + # Restore original PR author on cherry-pick for DCO compliance + await self._restore_original_author_for_cherry_pick( + pull_request=pull_request, + git_cmd=git_cmd, + github_token=github_token, + ) + # Push the branch rc, out, err = await run_command( command=push_command, diff --git a/webhook_server/tests/test_runner_handler.py b/webhook_server/tests/test_runner_handler.py index f0c729e1..bdfe3d89 100644 --- a/webhook_server/tests/test_runner_handler.py +++ b/webhook_server/tests/test_runner_handler.py @@ -1035,7 +1035,10 @@ async def test_cherry_pick_command_failure(self, runner_handler: RunnerHandler, async def test_cherry_pick_success(self, runner_handler: RunnerHandler, mock_pull_request: Mock) -> None: """Test cherry_pick with successful execution.""" runner_handler.github_webhook.pypi = {"token": "dummy"} - with patch.object(runner_handler, "is_branch_exists", new=AsyncMock(return_value=Mock())): + with ( + patch.object(runner_handler, "_restore_original_author_for_cherry_pick", new=AsyncMock(return_value=False)), + patch.object(runner_handler, "is_branch_exists", new=AsyncMock(return_value=Mock())), + ): with patch.object(runner_handler.check_run_handler, "set_check_in_progress") as mock_set_progress: with patch.object(runner_handler.check_run_handler, "set_check_success") as mock_set_success: with patch.object(runner_handler, "_checkout_worktree") as mock_checkout: @@ -1187,7 +1190,10 @@ async def test_cherry_pick_calls_checkout_worktree_with_skip_merge( ) -> None: """Test cherry_pick passes skip_merge=True to _checkout_worktree.""" runner_handler.github_webhook.pypi = {"token": "dummy"} - with patch.object(runner_handler, "is_branch_exists", new=AsyncMock(return_value=Mock())): + with ( + patch.object(runner_handler, "_restore_original_author_for_cherry_pick", new=AsyncMock(return_value=False)), + patch.object(runner_handler, "is_branch_exists", new=AsyncMock(return_value=Mock())), + ): with patch.object(runner_handler.check_run_handler, "set_check_in_progress"): with patch.object(runner_handler.check_run_handler, "set_check_success"): with patch.object(runner_handler, "_checkout_worktree") as mock_checkout: @@ -1336,7 +1342,10 @@ async def cherry_pick_setup( ) -> AsyncGenerator[CherryPickMocks]: """Common setup for cherry-pick tests.""" runner_handler.github_webhook.pypi = {"token": "dummy"} - with patch.object(runner_handler, "is_branch_exists", new=AsyncMock(return_value=Mock())): + with ( + patch.object(runner_handler, "_restore_original_author_for_cherry_pick", new=AsyncMock(return_value=False)), + patch.object(runner_handler, "is_branch_exists", new=AsyncMock(return_value=Mock())), + ): with patch.object(runner_handler.check_run_handler, "set_check_in_progress") as mock_set_progress: with patch.object(runner_handler.check_run_handler, "set_check_success") as mock_set_success: with patch.object(runner_handler, "_checkout_worktree") as mock_checkout: @@ -1570,7 +1579,10 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st return (True, "https://github.com/test-org/test-repo/pull/99", "") return (True, "success", "") - with patch.object(runner_handler, "is_branch_exists", new=AsyncMock(return_value=Mock())): + with ( + patch.object(runner_handler, "_restore_original_author_for_cherry_pick", new=AsyncMock(return_value=False)), + patch.object(runner_handler, "is_branch_exists", new=AsyncMock(return_value=Mock())), + ): with patch.object(runner_handler.check_run_handler, "set_check_in_progress"): with patch.object(runner_handler.check_run_handler, "set_check_success") as mock_set_success: with patch.object(runner_handler, "_checkout_worktree") as mock_checkout: @@ -1641,7 +1653,10 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st return (True, "https://github.com/test-org/test-repo/pull/99", "") return (True, "success", "") - with patch.object(runner_handler, "is_branch_exists", new=AsyncMock(return_value=Mock())): + with ( + patch.object(runner_handler, "_restore_original_author_for_cherry_pick", new=AsyncMock(return_value=False)), + patch.object(runner_handler, "is_branch_exists", new=AsyncMock(return_value=Mock())), + ): with patch.object(runner_handler.check_run_handler, "set_check_in_progress"): with patch.object(runner_handler.check_run_handler, "set_check_success") as mock_set_success: with patch.object(runner_handler, "_checkout_worktree") as mock_checkout: @@ -1889,6 +1904,282 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st mock_ai_cli.assert_not_called() +class TestRestoreOriginalAuthorForCherryPick: + """Test suite for _restore_original_author_for_cherry_pick method.""" + + @pytest.fixture + def mock_github_webhook(self) -> Mock: + """Create a mock GithubWebhook instance.""" + mock_webhook = Mock() + mock_webhook.hook_data = {"action": "opened"} + mock_webhook.logger = Mock() + mock_webhook.log_prefix = "[TEST]" + mock_webhook.repository = Mock() + mock_webhook.repository.clone_url = "https://github.com/test/repo.git" + mock_webhook.repository.owner.login = "test-owner" + mock_webhook.token = "test-token" # pragma: allowlist secret + mock_webhook.clone_repo_dir = "/tmp/test-repo" + mock_webhook.tox = {} + mock_webhook.pre_commit = False + mock_webhook.build_and_push_container = False + mock_webhook.pypi = {} + mock_webhook.conventional_title = "" + mock_webhook.container_repository_username = "" + mock_webhook.container_repository_password = "" # pragma: allowlist secret + mock_webhook.slack_webhook_url = "" + mock_webhook.repository_full_name = "test/repo" + mock_webhook.dockerfile = "" + mock_webhook.container_build_args = [] + mock_webhook.container_command_args = [] + mock_webhook.ctx = None + mock_webhook.custom_check_runs = [] + mock_webhook.ai_features = None + mock_webhook.config = Mock() + mock_webhook.config.get_value = Mock(return_value=None) + mock_webhook.mask_sensitive = False + return mock_webhook + + @pytest.fixture + def mock_owners_file_handler(self) -> Mock: + """Create a mock OwnersFileHandler instance.""" + return Mock() + + @pytest.fixture + def runner_handler(self, mock_github_webhook: Mock, mock_owners_file_handler: Mock) -> RunnerHandler: + """Create a RunnerHandler instance with mocked dependencies.""" + return RunnerHandler(mock_github_webhook, mock_owners_file_handler) + + @staticmethod + def _make_pr_with_commits(commit_messages: list[str]) -> Mock: + """Create a mock PullRequest with commits having the given messages.""" + mock_pr = Mock() + mock_commits = [] + for msg in commit_messages: + mock_commit = Mock() + mock_commit.commit.message = msg + mock_commits.append(mock_commit) + mock_pr.get_commits.return_value = mock_commits + return mock_pr + + @pytest.mark.asyncio + async def test_amend_when_email_mismatch(self, runner_handler: RunnerHandler) -> None: + """Original PR has Signed-off-by, cherry-pick author differs — amend.""" + mock_pr = self._make_pr_with_commits([ + "[Storage] Update owners\n\nSigned-off-by: Jenia Peimer \n" + ]) + + async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, str, str]: + if "log -1 --format=%ae" in command: + return (True, "86722603+jpeimer@users.noreply.github.com", "") + if "commit --amend" in command: + assert "Jenia Peimer " in command + return (True, "success", "") + return (True, "", "") + + with ( + patch( + "webhook_server.libs.handlers.runner_handler.run_command", + new=AsyncMock(side_effect=run_command_side_effect), + ), + patch( + "webhook_server.libs.handlers.runner_handler.github_api_call", + new=AsyncMock(side_effect=lambda fn, *a, **kw: fn() if callable(fn) else fn), + ), + ): + result = await runner_handler._restore_original_author_for_cherry_pick( + pull_request=mock_pr, + git_cmd="git --work-tree=/tmp/test --git-dir=/tmp/test/.git", + github_token="test-token", # pragma: allowlist secret + ) + assert result is True + + @pytest.mark.asyncio + async def test_no_amend_when_email_matches(self, runner_handler: RunnerHandler) -> None: + """Original PR has Signed-off-by matching cherry-pick author — no amend.""" + mock_pr = self._make_pr_with_commits(["feat: something\n\nSigned-off-by: Test User \n"]) + + async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, str, str]: + if "log -1 --format=%ae" in command: + return (True, "test@example.com", "") + return (True, "", "") + + with ( + patch( + "webhook_server.libs.handlers.runner_handler.run_command", + new=AsyncMock(side_effect=run_command_side_effect), + ), + patch( + "webhook_server.libs.handlers.runner_handler.github_api_call", + new=AsyncMock(side_effect=lambda fn, *a, **kw: fn() if callable(fn) else fn), + ), + ): + result = await runner_handler._restore_original_author_for_cherry_pick( + pull_request=mock_pr, + git_cmd="git --work-tree=/tmp/test --git-dir=/tmp/test/.git", + github_token="test-token", # pragma: allowlist secret + ) + assert result is False + + @pytest.mark.asyncio + async def test_no_signoff_in_original_pr(self, runner_handler: RunnerHandler) -> None: + """Original PR commits have no Signed-off-by — skip.""" + mock_pr = self._make_pr_with_commits(["feat: something\n\nNo sign-off here.\n"]) + + with patch( + "webhook_server.libs.handlers.runner_handler.github_api_call", + new=AsyncMock(side_effect=lambda fn, *a, **kw: fn() if callable(fn) else fn), + ): + result = await runner_handler._restore_original_author_for_cherry_pick( + pull_request=mock_pr, + git_cmd="git --work-tree=/tmp/test --git-dir=/tmp/test/.git", + github_token="test-token", # pragma: allowlist secret + ) + assert result is False + + @pytest.mark.asyncio + async def test_amend_failure(self, runner_handler: RunnerHandler) -> None: + """Amend command fails — return False without blocking cherry-pick.""" + mock_pr = self._make_pr_with_commits(["feat: test\n\nSigned-off-by: User \n"]) + + async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, str, str]: + if "log -1 --format=%ae" in command: + return (True, "noreply@github.com", "") + if "commit --amend" in command: + return (False, "", "error: could not amend") + return (True, "", "") + + with ( + patch( + "webhook_server.libs.handlers.runner_handler.run_command", + new=AsyncMock(side_effect=run_command_side_effect), + ), + patch( + "webhook_server.libs.handlers.runner_handler.github_api_call", + new=AsyncMock(side_effect=lambda fn, *a, **kw: fn() if callable(fn) else fn), + ), + ): + result = await runner_handler._restore_original_author_for_cherry_pick( + pull_request=mock_pr, + git_cmd="git --work-tree=/tmp/test --git-dir=/tmp/test/.git", + github_token="test-token", # pragma: allowlist secret + ) + assert result is False + + @pytest.mark.asyncio + async def test_no_commits_in_original_pr(self, runner_handler: RunnerHandler) -> None: + """Original PR has no commits — skip.""" + mock_pr = self._make_pr_with_commits([]) + + with patch( + "webhook_server.libs.handlers.runner_handler.github_api_call", + new=AsyncMock(side_effect=lambda fn, *a, **kw: fn() if callable(fn) else fn), + ): + result = await runner_handler._restore_original_author_for_cherry_pick( + pull_request=mock_pr, + git_cmd="git --work-tree=/tmp/test --git-dir=/tmp/test/.git", + github_token="test-token", # pragma: allowlist secret + ) + assert result is False + + @pytest.mark.asyncio + async def test_ae_read_failure_still_amends(self, runner_handler: RunnerHandler) -> None: + """Cannot read current author email — log warning and proceed to amend.""" + mock_pr = self._make_pr_with_commits(["feat: test\n\nSigned-off-by: User \n"]) + + async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, str, str]: + if "log -1 --format=%ae" in command: + return (False, "", "fatal: bad revision") + if "commit --amend" in command: + assert "User " in command + return (True, "success", "") + return (True, "", "") + + with ( + patch( + "webhook_server.libs.handlers.runner_handler.run_command", + new=AsyncMock(side_effect=run_command_side_effect), + ), + patch( + "webhook_server.libs.handlers.runner_handler.github_api_call", + new=AsyncMock(side_effect=lambda fn, *a, **kw: fn() if callable(fn) else fn), + ), + ): + result = await runner_handler._restore_original_author_for_cherry_pick( + pull_request=mock_pr, + git_cmd="git --work-tree=/tmp/test --git-dir=/tmp/test/.git", + github_token="test-token", # pragma: allowlist secret + ) + assert result is True + runner_handler.github_webhook.logger.warning.assert_called() + + @pytest.mark.asyncio + async def test_multiple_commits_finds_signoff_in_last(self, runner_handler: RunnerHandler) -> None: + """Multiple commits, only last has Signed-off-by — uses it.""" + mock_pr = self._make_pr_with_commits([ + "first commit without signoff", + "second commit\n\nSigned-off-by: Author Two \n", + ]) + + async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, str, str]: + if "log -1 --format=%ae" in command: + return (True, "noreply@github.com", "") + if "commit --amend" in command: + assert "Author Two " in command + return (True, "success", "") + return (True, "", "") + + with ( + patch( + "webhook_server.libs.handlers.runner_handler.run_command", + new=AsyncMock(side_effect=run_command_side_effect), + ), + patch( + "webhook_server.libs.handlers.runner_handler.github_api_call", + new=AsyncMock(side_effect=lambda fn, *a, **kw: fn() if callable(fn) else fn), + ), + ): + result = await runner_handler._restore_original_author_for_cherry_pick( + pull_request=mock_pr, + git_cmd="git --work-tree=/tmp/test --git-dir=/tmp/test/.git", + github_token="test-token", # pragma: allowlist secret + ) + assert result is True + + @pytest.mark.asyncio + async def test_multiple_signoffs_in_commit_uses_last(self, runner_handler: RunnerHandler) -> None: + """Single commit with multiple Signed-off-by trailers — uses last.""" + mock_pr = self._make_pr_with_commits([ + "feat: co-authored\n\n" + "Signed-off-by: First Author \n" + "Signed-off-by: Second Author \n", + ]) + + async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, str, str]: + if "log -1 --format=%ae" in command: + return (True, "noreply@github.com", "") + if "commit --amend" in command: + assert "Second Author " in command + return (True, "success", "") + return (True, "", "") + + with ( + patch( + "webhook_server.libs.handlers.runner_handler.run_command", + new=AsyncMock(side_effect=run_command_side_effect), + ), + patch( + "webhook_server.libs.handlers.runner_handler.github_api_call", + new=AsyncMock(side_effect=lambda fn, *a, **kw: fn() if callable(fn) else fn), + ), + ): + result = await runner_handler._restore_original_author_for_cherry_pick( + pull_request=mock_pr, + git_cmd="git --work-tree=/tmp/test --git-dir=/tmp/test/.git", + github_token="test-token", # pragma: allowlist secret + ) + assert result is True + + class TestCheckConfig: """Test suite for CheckConfig dataclass.""" From cff4cf951cfa287bc0de4f82695c26a4c51606f5 Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 16 Apr 2026 10:52:14 +0000 Subject: [PATCH 2/8] fix: address CodeRabbit review - best-effort try/except, redact author, 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 Signed-off-by: rnetser --- .../libs/handlers/runner_handler.py | 103 ++++++++++-------- webhook_server/tests/test_runner_handler.py | 7 +- 2 files changed, 63 insertions(+), 47 deletions(-) diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index 5014df3b..dc3a2b43 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -756,58 +756,69 @@ async def _restore_original_author_for_cherry_pick( Returns True if the commit was amended, False if no amendment was needed or possible. """ - # Get the original PR's commits to find the Signed-off-by - commits = await github_api_call( - lambda: list(pull_request.get_commits()), logger=self.logger, log_prefix=self.log_prefix - ) - if not commits: - self.logger.debug(f"{self.log_prefix} No commits found in original PR, skipping author restore") - return False + try: + # Get the original PR's commits to find the Signed-off-by + commits = await github_api_call( + lambda: list(pull_request.get_commits()), logger=self.logger, log_prefix=self.log_prefix + ) + if not commits: + self.logger.debug(f"{self.log_prefix} No commits found in original PR, skipping author restore") + return False - # Search original PR commits for a Signed-off-by trailer (check last commit first) - author_name: str | None = None - author_email: str | None = None - 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 + # Search original PR commits for a Signed-off-by trailer (check last commit first) + author_name: str | None = None + author_email: str | None = None + 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: + author_name, author_email = signoff_match[-1] + author_name = author_name.strip() + break + + if not author_name or not author_email: + self.logger.debug(f"{self.log_prefix} No Signed-off-by in original PR commits, skipping author restore") + return False + + # Check if the cherry-picked commit author already matches + rc, current_author_email, _ = await run_command( + command=f"{git_cmd} log -1 --format=%ae", + log_prefix=self.log_prefix, + redact_secrets=[github_token], + mask_sensitive=self.github_webhook.mask_sensitive, ) - signoff_match = re.findall(r"Signed-off-by:\s*(.+?)\s*<([^>]+)>", commit_msg) - if signoff_match: - author_name, author_email = signoff_match[-1] - author_name = author_name.strip() - break - - if not author_name or not author_email: - self.logger.debug(f"{self.log_prefix} No Signed-off-by in original PR commits, skipping author restore") - return False + if not rc: + self.logger.warning( + f"{self.log_prefix} Could not read current author email, proceeding with author amend" + ) + elif current_author_email.strip() == author_email: + self.logger.debug( + f"{self.log_prefix} Author email already matches original PR sign-off, no amend needed" + ) + return False - # Check if the cherry-picked commit author already matches - rc, current_author_email, _ = await run_command( - command=f"{git_cmd} log -1 --format=%ae", - log_prefix=self.log_prefix, - redact_secrets=[github_token], - mask_sensitive=self.github_webhook.mask_sensitive, - ) - if not rc: - self.logger.warning(f"{self.log_prefix} Could not read current author email, proceeding with author amend") - elif current_author_email.strip() == author_email: - self.logger.debug(f"{self.log_prefix} Author email already matches original PR sign-off, no amend needed") - return False + # Amend the commit author to match the original PR's Signed-off-by + author_spec = f"{author_name} <{author_email}>" + rc, _, err = await run_command( + command=f"{git_cmd} commit --amend --no-edit --author={shlex.quote(author_spec)}", + log_prefix=self.log_prefix, + redact_secrets=[github_token, author_spec, author_email, author_name], + mask_sensitive=self.github_webhook.mask_sensitive, + ) + if not rc: + self.logger.warning(f"{self.log_prefix} Failed to amend cherry-pick author for DCO compliance: {err}") + return False - # Amend the commit author to match the original PR's Signed-off-by - rc, _, err = await run_command( - command=f"{git_cmd} commit --amend --no-edit --author={shlex.quote(f'{author_name} <{author_email}>')}", - log_prefix=self.log_prefix, - redact_secrets=[github_token], - mask_sensitive=self.github_webhook.mask_sensitive, - ) - if not rc: - self.logger.warning(f"{self.log_prefix} Failed to amend cherry-pick author for DCO compliance: {err}") + self.logger.info(f"{self.log_prefix} Restored original author on cherry-pick for DCO compliance") + return True + except asyncio.CancelledError: + raise + except Exception: + self.logger.exception(f"{self.log_prefix} Failed to restore original author for cherry-pick") return False - self.logger.info(f"{self.log_prefix} Restored original author '{author_name} <{author_email}>' on cherry-pick") - return True - async def _resolve_cherry_pick_with_ai( self, worktree_path: str, diff --git a/webhook_server/tests/test_runner_handler.py b/webhook_server/tests/test_runner_handler.py index bdfe3d89..4c387d75 100644 --- a/webhook_server/tests/test_runner_handler.py +++ b/webhook_server/tests/test_runner_handler.py @@ -1036,7 +1036,11 @@ async def test_cherry_pick_success(self, runner_handler: RunnerHandler, mock_pul """Test cherry_pick with successful execution.""" runner_handler.github_webhook.pypi = {"token": "dummy"} 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())), ): with patch.object(runner_handler.check_run_handler, "set_check_in_progress") as mock_set_progress: @@ -1063,6 +1067,7 @@ async def test_cherry_pick_success(self, runner_handler: RunnerHandler, mock_pul ), ): await runner_handler.cherry_pick(mock_pull_request, "main") + mock_restore_author.assert_awaited_once() mock_set_progress.assert_called_once() mock_set_success.assert_called_once() # Called twice: success comment + label warning (mock URL can't parse PR number) From 57d8020ef8479fca29eedde0a8c18a9e82814d77 Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 16 Apr 2026 11:58:31 +0000 Subject: [PATCH 3/8] fix: redact stderr on amend failure, add exception/cancel tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 Signed-off-by: rnetser --- .../libs/handlers/runner_handler.py | 9 ++++- webhook_server/tests/test_runner_handler.py | 34 +++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index dc3a2b43..9696153d 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -808,7 +808,14 @@ async def _restore_original_author_for_cherry_pick( mask_sensitive=self.github_webhook.mask_sensitive, ) if not rc: - self.logger.warning(f"{self.log_prefix} Failed to amend cherry-pick author for DCO compliance: {err}") + redacted_err = _redact_secrets( + err, + [github_token, author_spec, author_email, author_name], + mask_sensitive=self.github_webhook.mask_sensitive, + ) + self.logger.warning( + f"{self.log_prefix} Failed to amend cherry-pick author for DCO compliance: {redacted_err}" + ) return False self.logger.info(f"{self.log_prefix} Restored original author on cherry-pick for DCO compliance") diff --git a/webhook_server/tests/test_runner_handler.py b/webhook_server/tests/test_runner_handler.py index 4c387d75..03572210 100644 --- a/webhook_server/tests/test_runner_handler.py +++ b/webhook_server/tests/test_runner_handler.py @@ -1,3 +1,4 @@ +import asyncio from collections.abc import AsyncGenerator, Generator from contextlib import asynccontextmanager from dataclasses import dataclass @@ -2184,6 +2185,39 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st ) assert result is True + @pytest.mark.asyncio + async def test_api_failure_returns_false(self, runner_handler: RunnerHandler) -> None: + """github_api_call raises exception — return False (best-effort, don't block cherry-pick).""" + mock_pr = self._make_pr_with_commits(["feat: test\n\nSigned-off-by: User \n"]) + + with patch( + "webhook_server.libs.handlers.runner_handler.github_api_call", + new=AsyncMock(side_effect=RuntimeError("GitHub API unavailable")), + ): + result = await runner_handler._restore_original_author_for_cherry_pick( + pull_request=mock_pr, + git_cmd="git --work-tree=/tmp/test --git-dir=/tmp/test/.git", + github_token="test-token", # pragma: allowlist secret + ) + assert result is False + runner_handler.github_webhook.logger.exception.assert_called() + + @pytest.mark.asyncio + async def test_cancelled_error_is_reraised(self, runner_handler: RunnerHandler) -> None: + """asyncio.CancelledError is re-raised, not swallowed.""" + mock_pr = self._make_pr_with_commits(["feat: test\n\nSigned-off-by: User \n"]) + + with patch( + "webhook_server.libs.handlers.runner_handler.github_api_call", + new=AsyncMock(side_effect=asyncio.CancelledError()), + ): + with pytest.raises(asyncio.CancelledError): + await runner_handler._restore_original_author_for_cherry_pick( + pull_request=mock_pr, + git_cmd="git --work-tree=/tmp/test --git-dir=/tmp/test/.git", + github_token="test-token", # pragma: allowlist secret + ) + class TestCheckConfig: """Test suite for CheckConfig dataclass.""" From 2ff1790a3fa63262df4e41c1d6af155f0f3b9d38 Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 16 Apr 2026 13:46:12 +0000 Subject: [PATCH 4/8] fix: use commit author identity instead of sign-off name for DCO Source author name and email from the original PR commit's git author (commit.commit.author) instead of parsing the Signed-off-by trailer name. This fixes cases where Signed-off-by has a username (e.g., rnetser) but DCO expects the full name (e.g., Ruth Netser) matching the GitHub profile. Additionally, amend the commit message to replace all Signed-off-by trailers with a correct one matching the restored author identity. Closes: https://github.com/myk-org/github-webhook-server/issues/1072 Assisted-by: Claude Signed-off-by: rnetser --- .../libs/handlers/runner_handler.py | 107 +++++++++--- webhook_server/tests/test_runner_handler.py | 155 ++++++++++++++---- 2 files changed, 199 insertions(+), 63 deletions(-) diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index 9696153d..7393ff57 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -4,6 +4,7 @@ import re import shlex import shutil +import tempfile from asyncio import Task from collections.abc import AsyncGenerator, Callable, Coroutine from dataclasses import dataclass @@ -741,7 +742,7 @@ async def _restore_original_author_for_cherry_pick( git_cmd: str, github_token: str, ) -> bool: - """Amend cherry-picked commit author to match the original PR's Signed-off-by. + """Amend cherry-picked commit to restore the original PR author for DCO compliance. GitHub squash-merges rewrite the author email to the noreply format (e.g., 86722603+user@users.noreply.github.com). When git cherry-pick @@ -749,15 +750,16 @@ async def _restore_original_author_for_cherry_pick( no longer matches the Signed-off-by trailer. This method checks the original PR's commits for a Signed-off-by trailer. - If found, it amends the cherry-picked commit's author to match, restoring - the original PR owner's identity for DCO compliance. + If found, it uses the original commit's git author identity (name and email) + to amend the cherry-picked commit's author and Signed-off-by trailer, + ensuring DCO compliance. Only amends if the original PR commits had a Signed-off-by. Returns True if the commit was amended, False if no amendment was needed or possible. """ try: - # Get the original PR's commits to find the Signed-off-by + # Get the original PR's commits to find one with a Signed-off-by commits = await github_api_call( lambda: list(pull_request.get_commits()), logger=self.logger, log_prefix=self.log_prefix ) @@ -765,52 +767,103 @@ async def _restore_original_author_for_cherry_pick( self.logger.debug(f"{self.log_prefix} No commits found in original PR, skipping author restore") return False - # Search original PR commits for a Signed-off-by trailer (check last commit first) - author_name: str | None = None - author_email: str | None = None + # Find the last commit with a Signed-off-by trailer, and use its git author + source_commit = None 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: - author_name, author_email = signoff_match[-1] - author_name = author_name.strip() + if re.search(r"Signed-off-by:\s*.+?\s*<[^>]+>", commit_msg): + source_commit = commit break - if not author_name or not author_email: + if not source_commit: self.logger.debug(f"{self.log_prefix} No Signed-off-by in original PR commits, skipping author restore") return False - # Check if the cherry-picked commit author already matches - rc, current_author_email, _ = await run_command( - command=f"{git_cmd} log -1 --format=%ae", + # Use the original commit's git author — the authoritative identity before squash-merge rewrite + author_name = await github_api_call( + lambda: source_commit.commit.author.name, logger=self.logger, log_prefix=self.log_prefix + ) + author_email = await github_api_call( + lambda: source_commit.commit.author.email, logger=self.logger, log_prefix=self.log_prefix + ) + + # 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=[github_token], mask_sensitive=self.github_webhook.mask_sensitive, ) if not rc: self.logger.warning( - f"{self.log_prefix} Could not read current author email, proceeding with author amend" + f"{self.log_prefix} Could not read current author info, proceeding with author amend" ) - elif current_author_email.strip() == author_email: - self.logger.debug( - f"{self.log_prefix} Author email already matches original PR sign-off, no amend needed" - ) - return False + 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 - # Amend the commit author to match the original PR's Signed-off-by - author_spec = f"{author_name} <{author_email}>" - rc, _, err = await run_command( - command=f"{git_cmd} commit --amend --no-edit --author={shlex.quote(author_spec)}", + # 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=[github_token, author_spec, author_email, author_name], + redact_secrets=[github_token], 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 + author_spec = f"{author_name} <{author_email}>" + redact_list = [github_token, author_spec, author_email, author_name] + + if amended_msg: + msg_file_path = "" + try: + with tempfile.NamedTemporaryFile( + mode="w", encoding="utf-8", suffix=".txt", delete=False + ) as msg_file: + msg_file.write(amended_msg) + msg_file_path = msg_file.name + + rc, _, err = await run_command( + command=( + f"{git_cmd} commit --amend" + f" --author={shlex.quote(author_spec)}" + f" -F {shlex.quote(msg_file_path)}" + ), + log_prefix=self.log_prefix, + redact_secrets=redact_list, + mask_sensitive=self.github_webhook.mask_sensitive, + ) + finally: + if msg_file_path: + os.unlink(msg_file_path) + else: + rc, _, err = await run_command( + command=f"{git_cmd} commit --amend --no-edit --author={shlex.quote(author_spec)}", + log_prefix=self.log_prefix, + redact_secrets=redact_list, + mask_sensitive=self.github_webhook.mask_sensitive, + ) + if not rc: redacted_err = _redact_secrets( err, - [github_token, author_spec, author_email, author_name], + redact_list, mask_sensitive=self.github_webhook.mask_sensitive, ) self.logger.warning( diff --git a/webhook_server/tests/test_runner_handler.py b/webhook_server/tests/test_runner_handler.py index 03572210..e07ed36b 100644 --- a/webhook_server/tests/test_runner_handler.py +++ b/webhook_server/tests/test_runner_handler.py @@ -1956,13 +1956,19 @@ def runner_handler(self, mock_github_webhook: Mock, mock_owners_file_handler: Mo return RunnerHandler(mock_github_webhook, mock_owners_file_handler) @staticmethod - def _make_pr_with_commits(commit_messages: list[str]) -> Mock: - """Create a mock PullRequest with commits having the given messages.""" + def _make_pr_with_commits( + commit_messages: list[str], + author_name: str = "Test User", + author_email: str = "test@example.com", + ) -> Mock: + """Create a mock PullRequest with commits having the given messages and author.""" mock_pr = Mock() mock_commits = [] for msg in commit_messages: mock_commit = Mock() mock_commit.commit.message = msg + mock_commit.commit.author.name = author_name + mock_commit.commit.author.email = author_email mock_commits.append(mock_commit) mock_pr.get_commits.return_value = mock_commits return mock_pr @@ -1970,13 +1976,17 @@ def _make_pr_with_commits(commit_messages: list[str]) -> Mock: @pytest.mark.asyncio async def test_amend_when_email_mismatch(self, runner_handler: RunnerHandler) -> None: """Original PR has Signed-off-by, cherry-pick author differs — amend.""" - mock_pr = self._make_pr_with_commits([ - "[Storage] Update owners\n\nSigned-off-by: Jenia Peimer \n" - ]) + mock_pr = self._make_pr_with_commits( + ["[Storage] Update owners\n\nSigned-off-by: Jenia Peimer \n"], + author_name="Jenia Peimer", + author_email="jpeimer@redhat.com", + ) async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, str, str]: - if "log -1 --format=%ae" in command: - return (True, "86722603+jpeimer@users.noreply.github.com", "") + if "log -1 --format=%an%n%ae" in command: + return (True, "jpeimer\n86722603+jpeimer@users.noreply.github.com", "") + if "log -1 --format=%B" in command: + return (True, "[Storage] Update owners\n\nSigned-off-by: jpeimer \n", "") if "commit --amend" in command: assert "Jenia Peimer " in command return (True, "success", "") @@ -2000,13 +2010,17 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st assert result is True @pytest.mark.asyncio - async def test_no_amend_when_email_matches(self, runner_handler: RunnerHandler) -> None: - """Original PR has Signed-off-by matching cherry-pick author — no amend.""" - mock_pr = self._make_pr_with_commits(["feat: something\n\nSigned-off-by: Test User \n"]) + async def test_no_amend_when_author_matches(self, runner_handler: RunnerHandler) -> None: + """Original PR author matches cherry-pick author (name and email) — no amend.""" + mock_pr = self._make_pr_with_commits( + ["feat: something\n\nSigned-off-by: Test User \n"], + author_name="Test User", + author_email="test@example.com", + ) async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, str, str]: - if "log -1 --format=%ae" in command: - return (True, "test@example.com", "") + if "log -1 --format=%an%n%ae" in command: + return (True, "Test User\ntest@example.com", "") return (True, "", "") with ( @@ -2045,11 +2059,17 @@ async def test_no_signoff_in_original_pr(self, runner_handler: RunnerHandler) -> @pytest.mark.asyncio async def test_amend_failure(self, runner_handler: RunnerHandler) -> None: """Amend command fails — return False without blocking cherry-pick.""" - mock_pr = self._make_pr_with_commits(["feat: test\n\nSigned-off-by: User \n"]) + mock_pr = self._make_pr_with_commits( + ["feat: test\n\nSigned-off-by: User \n"], + author_name="User", + author_email="user@example.com", + ) async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, str, str]: - if "log -1 --format=%ae" in command: - return (True, "noreply@github.com", "") + if "log -1 --format=%an%n%ae" in command: + return (True, "noreply\nnoreply@github.com", "") + if "log -1 --format=%B" in command: + return (True, "feat: test\n\nSigned-off-by: noreply \n", "") if "commit --amend" in command: return (False, "", "error: could not amend") return (True, "", "") @@ -2088,13 +2108,19 @@ async def test_no_commits_in_original_pr(self, runner_handler: RunnerHandler) -> assert result is False @pytest.mark.asyncio - async def test_ae_read_failure_still_amends(self, runner_handler: RunnerHandler) -> None: - """Cannot read current author email — log warning and proceed to amend.""" - mock_pr = self._make_pr_with_commits(["feat: test\n\nSigned-off-by: User \n"]) + async def test_author_info_read_failure_still_amends(self, runner_handler: RunnerHandler) -> None: + """Cannot read current author info — log warning and proceed to amend.""" + mock_pr = self._make_pr_with_commits( + ["feat: test\n\nSigned-off-by: User \n"], + author_name="User", + author_email="user@example.com", + ) async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, str, str]: - if "log -1 --format=%ae" in command: + if "log -1 --format=%an%n%ae" in command: return (False, "", "fatal: bad revision") + if "log -1 --format=%B" in command: + return (True, "feat: test\n\nSigned-off-by: noreply \n", "") if "commit --amend" in command: assert "User " in command return (True, "success", "") @@ -2120,15 +2146,23 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st @pytest.mark.asyncio async def test_multiple_commits_finds_signoff_in_last(self, runner_handler: RunnerHandler) -> None: - """Multiple commits, only last has Signed-off-by — uses it.""" - mock_pr = self._make_pr_with_commits([ - "first commit without signoff", - "second commit\n\nSigned-off-by: Author Two \n", - ]) + """Multiple commits, only last has Signed-off-by — uses its git author.""" + mock_pr = Mock() + commit1 = Mock() + commit1.commit.message = "first commit without signoff" + commit1.commit.author.name = "Author One" + commit1.commit.author.email = "one@example.com" + commit2 = Mock() + commit2.commit.message = "second commit\n\nSigned-off-by: Author Two \n" + commit2.commit.author.name = "Author Two" + commit2.commit.author.email = "two@example.com" + mock_pr.get_commits.return_value = [commit1, commit2] async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, str, str]: - if "log -1 --format=%ae" in command: - return (True, "noreply@github.com", "") + if "log -1 --format=%an%n%ae" in command: + return (True, "noreply\nnoreply@github.com", "") + if "log -1 --format=%B" in command: + return (True, "second commit\n\nSigned-off-by: noreply \n", "") if "commit --amend" in command: assert "Author Two " in command return (True, "success", "") @@ -2152,19 +2186,27 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st assert result is True @pytest.mark.asyncio - async def test_multiple_signoffs_in_commit_uses_last(self, runner_handler: RunnerHandler) -> None: - """Single commit with multiple Signed-off-by trailers — uses last.""" - mock_pr = self._make_pr_with_commits([ - "feat: co-authored\n\n" - "Signed-off-by: First Author \n" - "Signed-off-by: Second Author \n", - ]) + async def test_signoff_name_mismatch_uses_commit_author(self, runner_handler: RunnerHandler) -> None: + """Signed-off-by has username but commit author has full name — uses commit author.""" + mock_pr = self._make_pr_with_commits( + ["docs: update docs\n\nSigned-off-by: rnetser \n"], + author_name="Ruth Netser", + author_email="rnetser@redhat.com", + ) + + amend_commands: list[str] = [] async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, str, str]: - if "log -1 --format=%ae" in command: - return (True, "noreply@github.com", "") + if "log -1 --format=%an%n%ae" in command: + return (True, "rnetser\nrnetser@redhat.com", "") + if "log -1 --format=%B" in command: + return ( + True, + "docs: update docs\n\nSigned-off-by: rnetser \n", + "", + ) if "commit --amend" in command: - assert "Second Author " in command + amend_commands.append(command) return (True, "success", "") return (True, "", "") @@ -2184,6 +2226,47 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st github_token="test-token", # pragma: allowlist secret ) assert result is True + # Verify it used the commit author name, not the sign-off name + assert len(amend_commands) == 1 + assert "Ruth Netser " in amend_commands[0] + + @pytest.mark.asyncio + async def test_message_read_failure_amends_author_only(self, runner_handler: RunnerHandler) -> None: + """Cannot read commit message — fall back to amending author only (--no-edit).""" + mock_pr = self._make_pr_with_commits( + ["feat: test\n\nSigned-off-by: User \n"], + author_name="User", + author_email="user@example.com", + ) + + async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, str, str]: + if "log -1 --format=%an%n%ae" in command: + return (True, "noreply\nnoreply@github.com", "") + if "log -1 --format=%B" in command: + return (False, "", "fatal: could not read") + if "commit --amend" in command: + assert "--no-edit" in command + assert "User " in command + return (True, "success", "") + return (True, "", "") + + with ( + patch( + "webhook_server.libs.handlers.runner_handler.run_command", + new=AsyncMock(side_effect=run_command_side_effect), + ), + patch( + "webhook_server.libs.handlers.runner_handler.github_api_call", + new=AsyncMock(side_effect=lambda fn, *a, **kw: fn() if callable(fn) else fn), + ), + ): + result = await runner_handler._restore_original_author_for_cherry_pick( + pull_request=mock_pr, + git_cmd="git --work-tree=/tmp/test --git-dir=/tmp/test/.git", + github_token="test-token", # pragma: allowlist secret + ) + assert result is True + runner_handler.github_webhook.logger.warning.assert_called() @pytest.mark.asyncio async def test_api_failure_returns_false(self, runner_handler: RunnerHandler) -> None: From 5d13a2da26793d4165bf0abc713d4e86b6ba4c4a Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 16 Apr 2026 17:00:26 +0000 Subject: [PATCH 5/8] fix: redact author in all git log reads, mock asyncio.to_thread in tests - 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 Signed-off-by: rnetser --- .../libs/handlers/runner_handler.py | 10 +++--- webhook_server/tests/test_runner_handler.py | 36 +++++++++---------- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index 7393ff57..da4bd893 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -789,11 +789,14 @@ async def _restore_original_author_for_cherry_pick( lambda: source_commit.commit.author.email, logger=self.logger, log_prefix=self.log_prefix ) + author_spec = f"{author_name} <{author_email}>" + redact_list = [github_token, author_spec, author_email, author_name] + # 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=[github_token], + redact_secrets=redact_list, mask_sensitive=self.github_webhook.mask_sensitive, ) if not rc: @@ -810,7 +813,7 @@ async def _restore_original_author_for_cherry_pick( rc, current_msg, _ = await run_command( command=f"{git_cmd} log -1 --format=%B", log_prefix=self.log_prefix, - redact_secrets=[github_token], + redact_secrets=redact_list, mask_sensitive=self.github_webhook.mask_sensitive, ) amended_msg: str | None = None @@ -827,9 +830,6 @@ async def _restore_original_author_for_cherry_pick( amended_msg = "\n".join(filtered_lines) + "\n" # Amend the commit author and optionally the message - author_spec = f"{author_name} <{author_email}>" - redact_list = [github_token, author_spec, author_email, author_name] - if amended_msg: msg_file_path = "" try: diff --git a/webhook_server/tests/test_runner_handler.py b/webhook_server/tests/test_runner_handler.py index e07ed36b..5545c856 100644 --- a/webhook_server/tests/test_runner_handler.py +++ b/webhook_server/tests/test_runner_handler.py @@ -1998,8 +1998,8 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st new=AsyncMock(side_effect=run_command_side_effect), ), patch( - "webhook_server.libs.handlers.runner_handler.github_api_call", - new=AsyncMock(side_effect=lambda fn, *a, **kw: fn() if callable(fn) else fn), + "asyncio.to_thread", + new=AsyncMock(side_effect=lambda fn, *a, **kw: fn(*a, **kw) if a or kw else fn()), ), ): result = await runner_handler._restore_original_author_for_cherry_pick( @@ -2029,8 +2029,8 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st new=AsyncMock(side_effect=run_command_side_effect), ), patch( - "webhook_server.libs.handlers.runner_handler.github_api_call", - new=AsyncMock(side_effect=lambda fn, *a, **kw: fn() if callable(fn) else fn), + "asyncio.to_thread", + new=AsyncMock(side_effect=lambda fn, *a, **kw: fn(*a, **kw) if a or kw else fn()), ), ): result = await runner_handler._restore_original_author_for_cherry_pick( @@ -2046,8 +2046,8 @@ async def test_no_signoff_in_original_pr(self, runner_handler: RunnerHandler) -> mock_pr = self._make_pr_with_commits(["feat: something\n\nNo sign-off here.\n"]) with patch( - "webhook_server.libs.handlers.runner_handler.github_api_call", - new=AsyncMock(side_effect=lambda fn, *a, **kw: fn() if callable(fn) else fn), + "asyncio.to_thread", + new=AsyncMock(side_effect=lambda fn, *a, **kw: fn(*a, **kw) if a or kw else fn()), ): result = await runner_handler._restore_original_author_for_cherry_pick( pull_request=mock_pr, @@ -2080,8 +2080,8 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st new=AsyncMock(side_effect=run_command_side_effect), ), patch( - "webhook_server.libs.handlers.runner_handler.github_api_call", - new=AsyncMock(side_effect=lambda fn, *a, **kw: fn() if callable(fn) else fn), + "asyncio.to_thread", + new=AsyncMock(side_effect=lambda fn, *a, **kw: fn(*a, **kw) if a or kw else fn()), ), ): result = await runner_handler._restore_original_author_for_cherry_pick( @@ -2097,8 +2097,8 @@ async def test_no_commits_in_original_pr(self, runner_handler: RunnerHandler) -> mock_pr = self._make_pr_with_commits([]) with patch( - "webhook_server.libs.handlers.runner_handler.github_api_call", - new=AsyncMock(side_effect=lambda fn, *a, **kw: fn() if callable(fn) else fn), + "asyncio.to_thread", + new=AsyncMock(side_effect=lambda fn, *a, **kw: fn(*a, **kw) if a or kw else fn()), ): result = await runner_handler._restore_original_author_for_cherry_pick( pull_request=mock_pr, @@ -2132,8 +2132,8 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st new=AsyncMock(side_effect=run_command_side_effect), ), patch( - "webhook_server.libs.handlers.runner_handler.github_api_call", - new=AsyncMock(side_effect=lambda fn, *a, **kw: fn() if callable(fn) else fn), + "asyncio.to_thread", + new=AsyncMock(side_effect=lambda fn, *a, **kw: fn(*a, **kw) if a or kw else fn()), ), ): result = await runner_handler._restore_original_author_for_cherry_pick( @@ -2174,8 +2174,8 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st new=AsyncMock(side_effect=run_command_side_effect), ), patch( - "webhook_server.libs.handlers.runner_handler.github_api_call", - new=AsyncMock(side_effect=lambda fn, *a, **kw: fn() if callable(fn) else fn), + "asyncio.to_thread", + new=AsyncMock(side_effect=lambda fn, *a, **kw: fn(*a, **kw) if a or kw else fn()), ), ): result = await runner_handler._restore_original_author_for_cherry_pick( @@ -2216,8 +2216,8 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st new=AsyncMock(side_effect=run_command_side_effect), ), patch( - "webhook_server.libs.handlers.runner_handler.github_api_call", - new=AsyncMock(side_effect=lambda fn, *a, **kw: fn() if callable(fn) else fn), + "asyncio.to_thread", + new=AsyncMock(side_effect=lambda fn, *a, **kw: fn(*a, **kw) if a or kw else fn()), ), ): result = await runner_handler._restore_original_author_for_cherry_pick( @@ -2256,8 +2256,8 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st new=AsyncMock(side_effect=run_command_side_effect), ), patch( - "webhook_server.libs.handlers.runner_handler.github_api_call", - new=AsyncMock(side_effect=lambda fn, *a, **kw: fn() if callable(fn) else fn), + "asyncio.to_thread", + new=AsyncMock(side_effect=lambda fn, *a, **kw: fn(*a, **kw) if a or kw else fn()), ), ): result = await runner_handler._restore_original_author_for_cherry_pick( From 06de1658bbca00144617d182ac8058f3763e6bd0 Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 16 Apr 2026 17:56:00 +0000 Subject: [PATCH 6/8] fix: verify message file contents in test, improve multi-commit test - 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 Signed-off-by: rnetser --- webhook_server/tests/test_runner_handler.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/webhook_server/tests/test_runner_handler.py b/webhook_server/tests/test_runner_handler.py index 5545c856..9267cdde 100644 --- a/webhook_server/tests/test_runner_handler.py +++ b/webhook_server/tests/test_runner_handler.py @@ -1989,6 +1989,13 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st return (True, "[Storage] Update owners\n\nSigned-off-by: jpeimer \n", "") if "commit --amend" in command: assert "Jenia Peimer " in command + # Verify the message file contains the corrected Signed-off-by trailer + assert "-F" in command + msg_path = command.rsplit("-F ", 1)[1].strip().strip("'\"") + with open(msg_path, encoding="utf-8") as f: + msg_content = f.read() + assert "Signed-off-by: Jenia Peimer " in msg_content + assert "Signed-off-by: jpeimer " not in msg_content return (True, "success", "") return (True, "", "") @@ -2146,10 +2153,10 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st @pytest.mark.asyncio async def test_multiple_commits_finds_signoff_in_last(self, runner_handler: RunnerHandler) -> None: - """Multiple commits, only last has Signed-off-by — uses its git author.""" + """Multiple commits with sign-offs — uses the last commit's git author (newest-first).""" mock_pr = Mock() commit1 = Mock() - commit1.commit.message = "first commit without signoff" + commit1.commit.message = "first commit\n\nSigned-off-by: Author One \n" commit1.commit.author.name = "Author One" commit1.commit.author.email = "one@example.com" commit2 = Mock() @@ -2165,6 +2172,7 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st return (True, "second commit\n\nSigned-off-by: noreply \n", "") if "commit --amend" in command: assert "Author Two " in command + assert "Author One " not in command return (True, "success", "") return (True, "", "") From 2f3de864f7cf442d80fac3e8236347e412b8fa75 Mon Sep 17 00:00:00 2001 From: rnetser Date: Fri, 17 Apr 2026 06:53:50 +0000 Subject: [PATCH 7/8] fix: use merge commit for DCO author restore, fallback to PR commits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: rnetser --- .../libs/handlers/runner_handler.py | 119 +++++------ webhook_server/tests/test_runner_handler.py | 190 ++++++++++-------- 2 files changed, 163 insertions(+), 146 deletions(-) diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index da4bd893..cc75743c 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -4,7 +4,6 @@ import re import shlex import shutil -import tempfile from asyncio import Task from collections.abc import AsyncGenerator, Callable, Coroutine from dataclasses import dataclass @@ -736,6 +735,47 @@ async def is_branch_exists(self, branch: str) -> bool: return False raise + async def _find_signoff_source( + self, + pull_request: PullRequest, + ) -> tuple[Any, str | None]: + """Find a commit with a Signed-off-by trailer and return it with the sign-off email. + + Checks the merge commit first (squash-merge). If the merge commit has no + Signed-off-by, falls back to scanning the PR's individual commits in + reverse order (regular merge). + + Returns (source_commit, signoff_email) or (None, None) if no sign-off found. + """ + # Try the merge commit first (squash-merge case) + merge_sha = await github_api_call( + lambda: pull_request.merge_commit_sha, logger=self.logger, log_prefix=self.log_prefix + ) + if merge_sha: + merge_commit = await github_api_call( + self.github_webhook.repository.get_commit, merge_sha, logger=self.logger, log_prefix=self.log_prefix + ) + commit_msg = await github_api_call( + lambda: merge_commit.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 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] + + return None, None + async def _restore_original_author_for_cherry_pick( self, pull_request: PullRequest, @@ -749,45 +789,28 @@ async def _restore_original_author_for_cherry_pick( replays such a commit, the DCO check fails because the author email no longer matches the Signed-off-by trailer. - This method checks the original PR's commits for a Signed-off-by trailer. - If found, it uses the original commit's git author identity (name and email) - to amend the cherry-picked commit's author and Signed-off-by trailer, - ensuring DCO compliance. + This method first checks the merge commit (via ``pull_request.merge_commit_sha``) + for a Signed-off-by trailer (squash-merge case). If not found, it falls back + to scanning the PR's individual commits (regular merge case). - Only amends if the original PR commits had a Signed-off-by. + The author identity is built from the source commit's git author name + (preserved by GitHub) combined with the email from the Signed-off-by trailer. Returns True if the commit was amended, False if no amendment was needed or possible. """ try: - # Get the original PR's commits to find one with a Signed-off-by - commits = await github_api_call( - lambda: list(pull_request.get_commits()), logger=self.logger, log_prefix=self.log_prefix - ) - if not commits: - self.logger.debug(f"{self.log_prefix} No commits found in original PR, skipping author restore") + # Try merge commit first (squash-merge), fall back to PR commits (regular merge) + source_commit, signoff_email = await self._find_signoff_source(pull_request) + if not source_commit or not signoff_email: + self.logger.debug(f"{self.log_prefix} No Signed-off-by found, skipping author restore") return False - # Find the last commit with a Signed-off-by trailer, and use its git author - source_commit = None - 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 - ) - if re.search(r"Signed-off-by:\s*.+?\s*<[^>]+>", commit_msg): - source_commit = commit - break - - if not source_commit: - self.logger.debug(f"{self.log_prefix} No Signed-off-by in original PR commits, skipping author restore") - return False - - # Use the original commit's git author — the authoritative identity before squash-merge rewrite + # Author name from the source commit's git author (GitHub preserves the display name) + # Author email from the Signed-off-by trailer (commit email may be noreply) author_name = await github_api_call( lambda: source_commit.commit.author.name, logger=self.logger, log_prefix=self.log_prefix ) - author_email = await github_api_call( - lambda: source_commit.commit.author.email, logger=self.logger, log_prefix=self.log_prefix - ) + author_email = signoff_email author_spec = f"{author_name} <{author_email}>" redact_list = [github_token, author_spec, author_email, author_name] @@ -830,35 +853,13 @@ async def _restore_original_author_for_cherry_pick( amended_msg = "\n".join(filtered_lines) + "\n" # Amend the commit author and optionally the message - if amended_msg: - msg_file_path = "" - try: - with tempfile.NamedTemporaryFile( - mode="w", encoding="utf-8", suffix=".txt", delete=False - ) as msg_file: - msg_file.write(amended_msg) - msg_file_path = msg_file.name - - rc, _, err = await run_command( - command=( - f"{git_cmd} commit --amend" - f" --author={shlex.quote(author_spec)}" - f" -F {shlex.quote(msg_file_path)}" - ), - log_prefix=self.log_prefix, - redact_secrets=redact_list, - mask_sensitive=self.github_webhook.mask_sensitive, - ) - finally: - if msg_file_path: - os.unlink(msg_file_path) - else: - rc, _, err = await run_command( - command=f"{git_cmd} commit --amend --no-edit --author={shlex.quote(author_spec)}", - log_prefix=self.log_prefix, - redact_secrets=redact_list, - mask_sensitive=self.github_webhook.mask_sensitive, - ) + msg_flag = f"-m {shlex.quote(amended_msg)}" if amended_msg else "--no-edit" + rc, _, err = await run_command( + command=f"{git_cmd} commit --amend --author={shlex.quote(author_spec)} {msg_flag}", + log_prefix=self.log_prefix, + redact_secrets=redact_list, + mask_sensitive=self.github_webhook.mask_sensitive, + ) if not rc: redacted_err = _redact_secrets( diff --git a/webhook_server/tests/test_runner_handler.py b/webhook_server/tests/test_runner_handler.py index 9267cdde..4acc1b23 100644 --- a/webhook_server/tests/test_runner_handler.py +++ b/webhook_server/tests/test_runner_handler.py @@ -1956,31 +1956,33 @@ def runner_handler(self, mock_github_webhook: Mock, mock_owners_file_handler: Mo return RunnerHandler(mock_github_webhook, mock_owners_file_handler) @staticmethod - def _make_pr_with_commits( - commit_messages: list[str], + def _make_merged_pr( + merge_commit_msg: str, author_name: str = "Test User", - author_email: str = "test@example.com", - ) -> Mock: - """Create a mock PullRequest with commits having the given messages and author.""" + merge_commit_sha: str = "abc123", + ) -> tuple[Mock, Mock]: + """Create a mock PullRequest with a merge commit. + + Returns (mock_pr, mock_merge_commit) so tests can configure the + repository.get_commit() return value. + """ mock_pr = Mock() - mock_commits = [] - for msg in commit_messages: - mock_commit = Mock() - mock_commit.commit.message = msg - mock_commit.commit.author.name = author_name - mock_commit.commit.author.email = author_email - mock_commits.append(mock_commit) - mock_pr.get_commits.return_value = mock_commits - return mock_pr + mock_pr.merge_commit_sha = merge_commit_sha + + mock_merge_commit = Mock() + mock_merge_commit.commit.message = merge_commit_msg + mock_merge_commit.commit.author.name = author_name + + return mock_pr, mock_merge_commit @pytest.mark.asyncio async def test_amend_when_email_mismatch(self, runner_handler: RunnerHandler) -> None: - """Original PR has Signed-off-by, cherry-pick author differs — amend.""" - mock_pr = self._make_pr_with_commits( - ["[Storage] Update owners\n\nSigned-off-by: Jenia Peimer \n"], + """Merge commit has Signed-off-by, cherry-pick author differs — amend.""" + mock_pr, mock_merge_commit = self._make_merged_pr( + merge_commit_msg="[Storage] Update owners\n\nSigned-off-by: Jenia Peimer \n", author_name="Jenia Peimer", - author_email="jpeimer@redhat.com", ) + runner_handler.github_webhook.repository.get_commit.return_value = mock_merge_commit async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, str, str]: if "log -1 --format=%an%n%ae" in command: @@ -1989,13 +1991,10 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st return (True, "[Storage] Update owners\n\nSigned-off-by: jpeimer \n", "") if "commit --amend" in command: assert "Jenia Peimer " in command - # Verify the message file contains the corrected Signed-off-by trailer - assert "-F" in command - msg_path = command.rsplit("-F ", 1)[1].strip().strip("'\"") - with open(msg_path, encoding="utf-8") as f: - msg_content = f.read() - assert "Signed-off-by: Jenia Peimer " in msg_content - assert "Signed-off-by: jpeimer " not in msg_content + # Verify the -m message contains the corrected Signed-off-by trailer + assert "-m" in command + assert "Signed-off-by: Jenia Peimer " in command + assert "Signed-off-by: jpeimer " not in command return (True, "success", "") return (True, "", "") @@ -2018,12 +2017,12 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st @pytest.mark.asyncio async def test_no_amend_when_author_matches(self, runner_handler: RunnerHandler) -> None: - """Original PR author matches cherry-pick author (name and email) — no amend.""" - mock_pr = self._make_pr_with_commits( - ["feat: something\n\nSigned-off-by: Test User \n"], + """Merge commit author matches cherry-pick author (name and email) — no amend.""" + mock_pr, mock_merge_commit = self._make_merged_pr( + merge_commit_msg="feat: something\n\nSigned-off-by: Test User \n", author_name="Test User", - author_email="test@example.com", ) + runner_handler.github_webhook.repository.get_commit.return_value = mock_merge_commit async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, str, str]: if "log -1 --format=%an%n%ae" in command: @@ -2048,9 +2047,15 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st assert result is False @pytest.mark.asyncio - async def test_no_signoff_in_original_pr(self, runner_handler: RunnerHandler) -> None: - """Original PR commits have no Signed-off-by — skip.""" - mock_pr = self._make_pr_with_commits(["feat: something\n\nNo sign-off here.\n"]) + async def test_no_signoff_in_merge_commit(self, runner_handler: RunnerHandler) -> None: + """Merge commit has no Signed-off-by, PR commits also have none — skip.""" + mock_pr, mock_merge_commit = self._make_merged_pr( + merge_commit_msg="feat: something\n\nNo sign-off here.\n", + ) + runner_handler.github_webhook.repository.get_commit.return_value = mock_merge_commit + pr_commit = Mock() + pr_commit.commit.message = "feat: no sign-off" + mock_pr.get_commits.return_value = [pr_commit] with patch( "asyncio.to_thread", @@ -2066,11 +2071,11 @@ async def test_no_signoff_in_original_pr(self, runner_handler: RunnerHandler) -> @pytest.mark.asyncio async def test_amend_failure(self, runner_handler: RunnerHandler) -> None: """Amend command fails — return False without blocking cherry-pick.""" - mock_pr = self._make_pr_with_commits( - ["feat: test\n\nSigned-off-by: User \n"], + mock_pr, mock_merge_commit = self._make_merged_pr( + merge_commit_msg="feat: test\n\nSigned-off-by: User \n", author_name="User", - author_email="user@example.com", ) + runner_handler.github_webhook.repository.get_commit.return_value = mock_merge_commit async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, str, str]: if "log -1 --format=%an%n%ae" in command: @@ -2099,37 +2104,28 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st assert result is False @pytest.mark.asyncio - async def test_no_commits_in_original_pr(self, runner_handler: RunnerHandler) -> None: - """Original PR has no commits — skip.""" - mock_pr = self._make_pr_with_commits([]) - - with patch( - "asyncio.to_thread", - new=AsyncMock(side_effect=lambda fn, *a, **kw: fn(*a, **kw) if a or kw else fn()), - ): - result = await runner_handler._restore_original_author_for_cherry_pick( - pull_request=mock_pr, - git_cmd="git --work-tree=/tmp/test --git-dir=/tmp/test/.git", - github_token="test-token", # pragma: allowlist secret - ) - assert result is False + async def test_fallback_to_pr_commits(self, runner_handler: RunnerHandler) -> None: + """Merge commit has no Signed-off-by (regular merge) — falls back to PR commits.""" + # Merge commit without sign-off (regular merge message) + mock_pr = Mock() + mock_pr.merge_commit_sha = "merge123" + mock_merge_commit = Mock() + mock_merge_commit.commit.message = "Merge pull request #42 from user/branch" + runner_handler.github_webhook.repository.get_commit.return_value = mock_merge_commit - @pytest.mark.asyncio - async def test_author_info_read_failure_still_amends(self, runner_handler: RunnerHandler) -> None: - """Cannot read current author info — log warning and proceed to amend.""" - mock_pr = self._make_pr_with_commits( - ["feat: test\n\nSigned-off-by: User \n"], - author_name="User", - author_email="user@example.com", - ) + # PR commit with sign-off + pr_commit = Mock() + pr_commit.commit.message = "feat: add feature\n\nSigned-off-by: Test User \n" + pr_commit.commit.author.name = "Test User" + mock_pr.get_commits.return_value = [pr_commit] async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, str, str]: if "log -1 --format=%an%n%ae" in command: - return (False, "", "fatal: bad revision") + return (True, "noreply\nnoreply@github.com", "") if "log -1 --format=%B" in command: - return (True, "feat: test\n\nSigned-off-by: noreply \n", "") + return (True, "feat: add feature\n\nSigned-off-by: noreply \n", "") if "commit --amend" in command: - assert "User " in command + assert "Test User " in command return (True, "success", "") return (True, "", "") @@ -2149,30 +2145,47 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st github_token="test-token", # pragma: allowlist secret ) assert result is True - runner_handler.github_webhook.logger.warning.assert_called() @pytest.mark.asyncio - async def test_multiple_commits_finds_signoff_in_last(self, runner_handler: RunnerHandler) -> None: - """Multiple commits with sign-offs — uses the last commit's git author (newest-first).""" + async def test_no_signoff_anywhere(self, runner_handler: RunnerHandler) -> None: + """No Signed-off-by in merge commit or PR commits — skip.""" mock_pr = Mock() - commit1 = Mock() - commit1.commit.message = "first commit\n\nSigned-off-by: Author One \n" - commit1.commit.author.name = "Author One" - commit1.commit.author.email = "one@example.com" - commit2 = Mock() - commit2.commit.message = "second commit\n\nSigned-off-by: Author Two \n" - commit2.commit.author.name = "Author Two" - commit2.commit.author.email = "two@example.com" - mock_pr.get_commits.return_value = [commit1, commit2] + mock_pr.merge_commit_sha = "merge123" + mock_merge_commit = Mock() + mock_merge_commit.commit.message = "Merge pull request #42 from user/branch" + runner_handler.github_webhook.repository.get_commit.return_value = mock_merge_commit + + pr_commit = Mock() + pr_commit.commit.message = "feat: no sign-off here" + mock_pr.get_commits.return_value = [pr_commit] + + with patch( + "asyncio.to_thread", + new=AsyncMock(side_effect=lambda fn, *a, **kw: fn(*a, **kw) if a or kw else fn()), + ): + result = await runner_handler._restore_original_author_for_cherry_pick( + pull_request=mock_pr, + git_cmd="git --work-tree=/tmp/test --git-dir=/tmp/test/.git", + github_token="test-token", # pragma: allowlist secret + ) + assert result is False + + @pytest.mark.asyncio + async def test_author_info_read_failure_still_amends(self, runner_handler: RunnerHandler) -> None: + """Cannot read current author info — log warning and proceed to amend.""" + mock_pr, mock_merge_commit = self._make_merged_pr( + merge_commit_msg="feat: test\n\nSigned-off-by: User \n", + author_name="User", + ) + runner_handler.github_webhook.repository.get_commit.return_value = mock_merge_commit async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, str, str]: if "log -1 --format=%an%n%ae" in command: - return (True, "noreply\nnoreply@github.com", "") + return (False, "", "fatal: bad revision") if "log -1 --format=%B" in command: - return (True, "second commit\n\nSigned-off-by: noreply \n", "") + return (True, "feat: test\n\nSigned-off-by: noreply \n", "") if "commit --amend" in command: - assert "Author Two " in command - assert "Author One " not in command + assert "User " in command return (True, "success", "") return (True, "", "") @@ -2192,15 +2205,16 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st github_token="test-token", # pragma: allowlist secret ) assert result is True + runner_handler.github_webhook.logger.warning.assert_called() @pytest.mark.asyncio - async def test_signoff_name_mismatch_uses_commit_author(self, runner_handler: RunnerHandler) -> None: - """Signed-off-by has username but commit author has full name — uses commit author.""" - mock_pr = self._make_pr_with_commits( - ["docs: update docs\n\nSigned-off-by: rnetser \n"], + async def test_signoff_name_mismatch_uses_merge_commit_author(self, runner_handler: RunnerHandler) -> None: + """Signed-off-by has username but merge commit author has full name — uses merge commit author.""" + mock_pr, mock_merge_commit = self._make_merged_pr( + merge_commit_msg="docs: update docs\n\nSigned-off-by: rnetser \n", author_name="Ruth Netser", - author_email="rnetser@redhat.com", ) + runner_handler.github_webhook.repository.get_commit.return_value = mock_merge_commit amend_commands: list[str] = [] @@ -2234,18 +2248,18 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st github_token="test-token", # pragma: allowlist secret ) assert result is True - # Verify it used the commit author name, not the sign-off name + # Verify it used the merge commit author name, not the sign-off name assert len(amend_commands) == 1 assert "Ruth Netser " in amend_commands[0] @pytest.mark.asyncio async def test_message_read_failure_amends_author_only(self, runner_handler: RunnerHandler) -> None: """Cannot read commit message — fall back to amending author only (--no-edit).""" - mock_pr = self._make_pr_with_commits( - ["feat: test\n\nSigned-off-by: User \n"], + mock_pr, mock_merge_commit = self._make_merged_pr( + merge_commit_msg="feat: test\n\nSigned-off-by: User \n", author_name="User", - author_email="user@example.com", ) + runner_handler.github_webhook.repository.get_commit.return_value = mock_merge_commit async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, str, str]: if "log -1 --format=%an%n%ae" in command: @@ -2279,7 +2293,8 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st @pytest.mark.asyncio async def test_api_failure_returns_false(self, runner_handler: RunnerHandler) -> None: """github_api_call raises exception — return False (best-effort, don't block cherry-pick).""" - mock_pr = self._make_pr_with_commits(["feat: test\n\nSigned-off-by: User \n"]) + mock_pr = Mock() + mock_pr.merge_commit_sha = "abc123" with patch( "webhook_server.libs.handlers.runner_handler.github_api_call", @@ -2296,7 +2311,8 @@ async def test_api_failure_returns_false(self, runner_handler: RunnerHandler) -> @pytest.mark.asyncio async def test_cancelled_error_is_reraised(self, runner_handler: RunnerHandler) -> None: """asyncio.CancelledError is re-raised, not swallowed.""" - mock_pr = self._make_pr_with_commits(["feat: test\n\nSigned-off-by: User \n"]) + mock_pr = Mock() + mock_pr.merge_commit_sha = "abc123" with patch( "webhook_server.libs.handlers.runner_handler.github_api_call", From e6b5ad856dba020612112ffd1c886124a3a6f019 Mon Sep 17 00:00:00 2001 From: rnetser Date: Sun, 19 Apr 2026 07:31:51 +0000 Subject: [PATCH 8/8] fix: anchor sign-off regex, normalize trailer on matching author, harden tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Anchor Signed-off-by regex to line boundaries with (?m)^...$ - Do not return early when author matches — still normalize the Signed-off-by trailer to ensure DCO compliance - Add author_email to _make_merged_pr and all test mocks - Assert _restore_original_author_for_cherry_pick awaited in AI cherry-pick tests (both conflict-resolution paths) Assisted-by: Claude Signed-off-by: rnetser --- .../libs/handlers/runner_handler.py | 16 +++++++---- webhook_server/tests/test_runner_handler.py | 28 ++++++++++++++++--- 2 files changed, 35 insertions(+), 9 deletions(-) diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index cc75743c..1e5e1810 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -758,7 +758,7 @@ async def _find_signoff_source( commit_msg = await github_api_call( lambda: merge_commit.commit.message, logger=self.logger, log_prefix=self.log_prefix ) - signoff_match = re.findall(r"Signed-off-by:\s*(.+?)\s*<([^>]+)>", commit_msg) + signoff_match = re.findall(r"(?m)^Signed-off-by:\s*(.+?)\s*<([^>\n]+)>\s*$", commit_msg) if signoff_match: return merge_commit, signoff_match[-1][1] @@ -770,7 +770,7 @@ async def _find_signoff_source( 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) + signoff_match = re.findall(r"(?m)^Signed-off-by:\s*(.+?)\s*<([^>\n]+)>\s*$", commit_msg) if signoff_match: return commit, signoff_match[-1][1] @@ -816,6 +816,7 @@ async def _restore_original_author_for_cherry_pick( redact_list = [github_token, author_spec, author_email, author_name] # Check if the cherry-picked commit author already matches (both name and email) + needs_author_amend = True rc, current_author_info, _ = await run_command( command=f"{git_cmd} log -1 --format=%an%n%ae", log_prefix=self.log_prefix, @@ -829,8 +830,7 @@ async def _restore_original_author_for_cherry_pick( 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( @@ -839,6 +839,7 @@ async def _restore_original_author_for_cherry_pick( redact_secrets=redact_list, mask_sensitive=self.github_webhook.mask_sensitive, ) + needs_message_amend = False amended_msg: str | None = None if not rc: self.logger.warning(f"{self.log_prefix} Could not read commit message, amending author only") @@ -851,9 +852,14 @@ async def _restore_original_author_for_cherry_pick( 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" rc, _, err = await run_command( command=f"{git_cmd} commit --amend --author={shlex.quote(author_spec)} {msg_flag}", log_prefix=self.log_prefix, diff --git a/webhook_server/tests/test_runner_handler.py b/webhook_server/tests/test_runner_handler.py index 4acc1b23..9a4463d7 100644 --- a/webhook_server/tests/test_runner_handler.py +++ b/webhook_server/tests/test_runner_handler.py @@ -1586,7 +1586,11 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st return (True, "success", "") 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())), ): with patch.object(runner_handler.check_run_handler, "set_check_in_progress"): @@ -1614,6 +1618,7 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st return_value=None, ): await runner_handler.cherry_pick(mock_pull_request, "main") + mock_restore_author.assert_awaited_once() mock_set_success.assert_called_once() mock_ai_cli.assert_called_once() # Verify prompt includes delete/modify conflict guidance @@ -1660,7 +1665,11 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st return (True, "success", "") 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())), ): with patch.object(runner_handler.check_run_handler, "set_check_in_progress"): @@ -1688,6 +1697,7 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st return_value=None, ): await runner_handler.cherry_pick(mock_pull_request, "main") + mock_restore_author.assert_awaited_once() mock_set_success.assert_called_once() mock_ai_cli.assert_called_once() # Verify cherry-pick --continue was NOT called @@ -1959,6 +1969,7 @@ def runner_handler(self, mock_github_webhook: Mock, mock_owners_file_handler: Mo 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]: """Create a mock PullRequest with a merge commit. @@ -1972,6 +1983,7 @@ def _make_merged_pr( 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 return mock_pr, mock_merge_commit @@ -1981,6 +1993,7 @@ async def test_amend_when_email_mismatch(self, runner_handler: RunnerHandler) -> mock_pr, mock_merge_commit = self._make_merged_pr( merge_commit_msg="[Storage] Update owners\n\nSigned-off-by: Jenia Peimer \n", author_name="Jenia Peimer", + author_email="jpeimer@redhat.com", ) runner_handler.github_webhook.repository.get_commit.return_value = mock_merge_commit @@ -2016,8 +2029,8 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st assert result is True @pytest.mark.asyncio - async def test_no_amend_when_author_matches(self, runner_handler: RunnerHandler) -> None: - """Merge commit author matches cherry-pick author (name and email) — no amend.""" + async def test_no_amend_when_author_and_signoff_match(self, runner_handler: RunnerHandler) -> None: + """Author and Signed-off-by both match — no amend needed.""" mock_pr, mock_merge_commit = self._make_merged_pr( merge_commit_msg="feat: something\n\nSigned-off-by: Test User \n", author_name="Test User", @@ -2027,6 +2040,8 @@ async def test_no_amend_when_author_matches(self, runner_handler: RunnerHandler) async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, str, str]: if "log -1 --format=%an%n%ae" in command: return (True, "Test User\ntest@example.com", "") + if "log -1 --format=%B" in command: + return (True, "feat: something\n\nSigned-off-by: Test User \n", "") return (True, "", "") with ( @@ -2074,6 +2089,7 @@ async def test_amend_failure(self, runner_handler: RunnerHandler) -> None: mock_pr, mock_merge_commit = self._make_merged_pr( merge_commit_msg="feat: test\n\nSigned-off-by: User \n", author_name="User", + author_email="user@example.com", ) runner_handler.github_webhook.repository.get_commit.return_value = mock_merge_commit @@ -2117,6 +2133,7 @@ async def test_fallback_to_pr_commits(self, runner_handler: RunnerHandler) -> No pr_commit = Mock() pr_commit.commit.message = "feat: add feature\n\nSigned-off-by: Test User \n" pr_commit.commit.author.name = "Test User" + pr_commit.commit.author.email = "test@example.com" mock_pr.get_commits.return_value = [pr_commit] async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, str, str]: @@ -2176,6 +2193,7 @@ async def test_author_info_read_failure_still_amends(self, runner_handler: Runne mock_pr, mock_merge_commit = self._make_merged_pr( merge_commit_msg="feat: test\n\nSigned-off-by: User \n", author_name="User", + author_email="user@example.com", ) runner_handler.github_webhook.repository.get_commit.return_value = mock_merge_commit @@ -2213,6 +2231,7 @@ async def test_signoff_name_mismatch_uses_merge_commit_author(self, runner_handl mock_pr, mock_merge_commit = self._make_merged_pr( merge_commit_msg="docs: update docs\n\nSigned-off-by: rnetser \n", author_name="Ruth Netser", + author_email="rnetser@redhat.com", ) runner_handler.github_webhook.repository.get_commit.return_value = mock_merge_commit @@ -2258,6 +2277,7 @@ async def test_message_read_failure_amends_author_only(self, runner_handler: Run mock_pr, mock_merge_commit = self._make_merged_pr( merge_commit_msg="feat: test\n\nSigned-off-by: User \n", author_name="User", + author_email="user@example.com", ) runner_handler.github_webhook.repository.get_commit.return_value = mock_merge_commit