feat: skip all CI checks and comment once when PR has merge conflicts - #1211
feat: skip all CI checks and comment once when PR has merge conflicts#1211rnetser wants to merge 5 commits into
Conversation
When a pushed or opened PR has merge conflicts, running CI wastes compute and gives the author no clear signal. This adds a guard at the top of process_opened_or_synchronize_pull_request: when the PR has conflicts (mergeable is False), skip all CI checks and return early. - Add has-conflicts label if absent and post a one-time comment (gated on the label absent->present transition and on _add_label succeeding) so there is no spam on repeated conflicting pushes - Reset review-attribution labels (approved/commented/changes-requested/lgtm) as a normal new commit would - New _pull_request_has_conflicts() helper reuses the mergeable None-poll logic; unknown/timeout/API-failure fail open (return False) so CI is never blocked on an indeterminate GitHub state - CI resumes automatically once conflicts are resolved on the next push Fixes #1208 Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: rnetser <rnetser@redhat.com>
|
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
Branch Management
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
Security Checks
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
PR Summary by QodoSkip CI and notify once for conflicted pull requests
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1. Cleanup failure resumes Oracle
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Extract shared _get_definitive_mergeable() used by both the conflict guard and label_pull_request_by_merge_state (removes duplicated mergeable None-poll). - Wrap pull_request.number in github_api_call (rule violation) - Bound the mergeable poll to a single 30s deadline; retry each refresh individually instead of the whole window - Skip test-oracle scheduling for conflicted PRs (process_opened_or_synchronize_pull_request now returns a conflict-skip bool consumed by callers) - Dedup the conflict comment via a hidden marker comment instead of the has-conflicts label, so a disabled label category or a prior failed comment no longer suppresses or blocks retry of the notification - Run mandatory review-label reset + ctx bookkeeping best-effort in all cases even if the optional label/comment notification raises - Patch asyncio.to_thread in conflict tests to avoid spawning real worker threads Refs #1208 Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: rnetser <rnetser@redhat.com>
|
/qodo The following review comments were reviewed and a decision was made:
|
The update addresses findings 1–5, 7, and 8 as described. However, finding 6 remains active: checking for |
|
Code review by qodo was updated up to the latest commit 7e50890 |
Address follow-up Qodo review findings on PR #1211. - Reuse TimeoutSampler (single bounded 30s window, sleep 5) run once via asyncio.to_thread with an exceptions_dict for transient GitHub 5xx/connection errors, instead of a hand-rolled deadline loop; keeps one bounded budget without github_api_call retry multiplication - Validate the conflict-comment dedup marker against trusted bot identities (security_trusted_committers + app_bot_login); a marker in an untrusted user comment no longer suppresses the authoritative notification (prevents marker spoofing). Empty trusted set validates no markers (fail-safe, never trust all authors) Refs #1208 Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: rnetser <rnetser@redhat.com>
|
Code review by qodo was updated up to the latest commit 5cf6638 |
Address Qodo finding that the TimeoutSampler poll callback accessed repository.get_pull().mergeable directly, bypassing the standardized retry wrapper. - Keep TimeoutSampler for the single bounded 30s window (no whole-window retry multiplication) - Route each individual mergeable refresh through github_api_call via an asyncio.run bridge inside the sampler's worker-thread callback, so every get_pull()/.mergeable access uses standardized retry - Drop the redundant sampler exceptions_dict and now-unused imports (github_api_call is the single retry authority) Refs #1208 Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: rnetser <rnetser@redhat.com>
|
Code review by qodo was updated up to the latest commit 646a7e2 |
…n-safe Address Qodo finding that the nested asyncio.run(github_api_call) poll bridge could not be cancelled and kept issuing GitHub requests after task exit. - Replace TimeoutSampler + asyncio.run bridge in _get_definitive_mergeable with a native async deadline loop: per-refresh github_api_call (standardized retry), single bounded 30s budget, cancellation propagates through await - Split the 178-line process_opened_or_synchronize_pull_request into _handle_conflicted_pull_request, _queue_pull_request_setup_tasks, and _run_pull_request_ci_tasks; orchestrator is now a short dispatcher (behavior unchanged) - Drop the unneeded [[tool.mypy.overrides]] for timeout_sampler (import removed; pyproject matches main) Refs #1208 Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: rnetser <rnetser@redhat.com>
| # (approved/commented/changes-requested/lgtm) as a normal new commit would. On non-clean | ||
| # synchronize this may run alongside the caller's reset; duplicate removals are benign and | ||
| # idempotent. | ||
| await self.remove_labels_when_pull_request_sync(pull_request=pull_request) |
There was a problem hiding this comment.
1. Cleanup failure resumes oracle 🐞 Bug ☼ Reliability
If remove_labels_when_pull_request_sync() fails while processing a conflicted PR, _handle_conflicted_pull_request() propagates the exception instead of preserving the successful conflict decision. The caller then interprets the exception as skipped_due_to_conflicts=False and schedules Test Oracle, violating the all-CI skip guarantee.
Agent Prompt
## Issue description
A failure while removing stale review labels propagates from the conflicted-PR path. The webhook caller consequently loses the `True` skip result and may schedule Test Oracle even though conflicts were definitively detected.
## Issue Context
Notification failures are already best-effort, but the subsequent cleanup operation is not. Preserve cancellation while treating ordinary cleanup failures as best-effort so the method still reaches the conflict completion path and returns the skip result.
## Fix Focus Areas
- webhook_server/libs/handlers/pull_request_handler.py[1238-1248]
- webhook_server/libs/handlers/pull_request_handler.py[1357-1366]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 5fedacc |
2 participants
Problem
When a pushed/opened PR has merge conflicts, the webhook server still queues and runs all CI checks. Running CI against an unmergeable state wastes compute and gives the author no clear signal.
What changed
process_opened_or_synchronize_pull_request: ifmergeable is False, skip ALL CI and return early.has-conflictslabel if absent and post a one-time comment (dedup via label absent->present; gated on_add_labelsucceeding)._pull_request_has_conflicts()helper; unknown/timeout/API-failure fail open so CI is never blocked on an indeterminate GitHub state.Testing
1770 passed, 90.17% coverage; ruff + mypy clean.
Review
Internal quality/guidelines/security/docs/spec reviewers all approved.
Fixes #1208