feat: cherry-pick retry/rebase commands + pre-commit auto-fix + security hardening#1108
Conversation
Code Review by Qodo
Context used 1.
|
|
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. |
|
Code review by qodo was updated up to the latest commit e3e4e57 |
|
@qodo-code-review[bot] The following review comments were reviewed and a decision was made:
|
|
Code review by qodo was updated up to the latest commit 86510ca |
|
Code review by qodo was updated up to the latest commit eaafc8a |
|
Code review by qodo was updated up to the latest commit 9c00066 |
|
Code review by qodo was updated up to the latest commit f1d8263 |
|
@qodo-code-review[bot] The following review comments were reviewed and a decision was made:
|
|
Code review by qodo was updated up to the latest commit 141108d |
… rebase - Store GitHub App bot login on GithubWebhook during init (github_api.py) - Cherry-pick retry: check PR author login matches app_bot_login instead of user.type - Rebase: check PR author login matches app_bot_login instead of user.type + label heuristic - Prevents false matches on Renovate, Dependabot, or other bot PRs
- Replace blocking github_app_api.get_user().login in __init__ with async github_api_call in process() - Log warning on failure instead of silent empty fallback - Guard with "if not self.app_bot_login" to avoid redundant API calls
…ncelledError, log missing API - Use logger.exception instead of logger.warning to preserve traceback - Add except asyncio.CancelledError: raise before broad except - Log debug message when GitHub App API is not available
…author check on empty app_bot_login - github_api.py: get_repository_github_app_api() does file I/O (reads private key) and GitHub App setup — was called synchronously in async process(), blocking the event loop. Now wrapped with github_api_call() for non-blocking + retry. - issue_comment_handler.py: when app_bot_login is empty (init failed or fallback token used), the author check 'pr_author_login != app_bot_login' would skip ALL PRs since no login matches empty string. Now guards the author check behind 'if self.github_webhook.app_bot_login', falling back to title+body match only.
- issue_comment_handler.py: add early-exit guard before the get_pulls loop in cherry-pick retry. If app_bot_login is empty, log ERROR and post a PR comment explaining the failure, then return. Reverts the previous silent fallback that skipped the author check. - runner_handler.py: add early-exit guard in rebase_pr() before the is_bot_pr check. If app_bot_login is empty, log ERROR and post a PR comment, then return. Without this, bot PRs would be treated as regular PRs (anyone could rebase) since empty string never matches any login. No silent fallbacks — if we cannot verify PR ownership, we stop.
- runner_handler.py rebase_pr(): remove top-level abort that blocked ALL rebases when app_bot_login was empty (including normal user PRs). Now: if app_bot_login is set, classify bot PRs normally; if not set, treat PR as user-owned with a warning log. Safe because is_user_valid_to_run_commands already gates access to the /rebase command. - issue_comment_handler.py cherry-pick retry: remove top-level abort that blocked the entire retry flow. Move the guard inside the loop after title match — if app_bot_login is empty, break out of the close loop with an error log (don't close any PR we can't verify ownership of), but still proceed to re-run the cherry-pick.
…e git refs, lazy PR iteration - issue_comment_handler.py: replace hook_data["issue"]["pull_request"]["merged_at"] with `await github_api_call(lambda: pull_request.merged, ...)` at both cherry-pick (line 578) and cherry-pick-retry (line 644) merge checks. Direct PR object access instead of defensive .get() chaining on webhook payload. - runner_handler.py rebase_pr(): quote all interpolated git refs (head_ref, base_ref) and worktree_path with shlex.quote() to prevent shlex.split() breakage on branch names with spaces or special chars. Also quote worktree_path in cherry_pick() pre-commit command. - issue_comment_handler.py cherry-pick retry: remove list() wrapper from get_pulls(state="open") to iterate lazily via PyGithub's PaginatedList, fetching pages on demand and breaking on first match. - test_issue_comment_handler.py: update all tests to set mock_pull_request.merged instead of manipulating hook_data["issue"]["pull_request"]["merged_at"].
… access - Convert get_pulls(state="open") back to list() inside github_api_call() to avoid blocking event loop on PaginatedList page boundary fetches. - Remove github_api_call lambda wrappers on .title, .user.login, .body — after list() materialization these properties are cached and safe to access directly. - Restore "found N open PRs to scan" debug log line.
b7eb792 to
e5044af
Compare
|
Code review by qodo was updated up to the latest commit e5044af |
|
@qodo-code-review[bot] The following review comments were reviewed and a decision was made:
|
|
@qodo-code-review[bot] The following review comments were reviewed and a decision was made:
|
…uth message - issue_comment_handler.py: add explicit empty-string check after .strip() in process_cherry_pick_retry_command. Whitespace-only args like "/cherry-pick-retry " bypass the shared arg parser check since whitespace is truthy, but strip to empty string. - owners_files_handler.py: change "not allowed to run retest commands" to "not allowed to run commands" — the method is now used for /cherry-pick, /rebase, /verified, /wip, not just retest. - test_owners_files_handler.py: update assertion to match new message.
Looking for bugs?Check back in a few minutes. An AI review agent is analyzing this pull request. |
|
@qodo-code-review[bot] The following review comments were reviewed and a decision was made:
|
|
@qodo-code-review[bot] The following review comments were reviewed and a decision was made:
|
|
@qodo-code-review[bot] The following review comments were reviewed and a decision was made:
|
2 similar comments
|
@qodo-code-review[bot] The following review comments were reviewed and a decision was made:
|
|
@qodo-code-review[bot] The following review comments were reviewed and a decision was made:
|
|
New container for ghcr.io/myk-org/github-webhook-server:latest published |
PR Summary by Qodo
Add cherry-pick retry + PR rebase commands, pre-commit auto-fix, and command guards
✨ Enhancement🐞 Bug fix🧪 Tests📝 Documentation🕐 40+ MinutesWalkthroughs
User Description
Summary
New commands (#1103)
/cherry-pick-retry <branch>— Retry a failed cherry-pick on merged PRs/rebase— Rebase any open PR onto its base branch (with bot-PR ownership validation)Pre-commit auto-fix (#1089)
Security hardening
is_user_valid_to_run_commandsguard to all 7 previously unguarded commands:/cherry-pick,/assign-reviewer,/assign-reviewers,/check-can-merge,/verified,/wip,/test-oracleCloses #1103
Closes #1089
AI Description
Diagram
graph TD A["Issue comment"] --> B["IssueCommentHandler"] --> C["OwnersFileHandler"] B --> D["RunnerHandler"] --> E["Local git worktree"] --> F{{"GitHub API/remote"}} D --> G["Pre-commit"] --> E B --> H["PullRequestHandler"]High-Level Assessment
The following are alternative approaches to this PR:
1. Use GitHub's built-in 'Update branch' / merge-upstream flow
2. Centralize command authorization via a dispatcher/decorator
3. Run formatting fixes only in CI and comment results (no auto-commit)
Recommendation: The PR’s approach is appropriate for a bot-driven maintenance workflow: rebase and cherry-pick actions need local git to be deterministic, and the added authorization gates materially reduce abuse risk. Consider a follow-up to centralize authorization in the command dispatcher to prevent future unguarded commands and reduce repetition.
File Changes
Enhancement (3)
Tests (2)
Documentation (1)