feat: block can-be-merged when unresolved review conversations exist#998
Open
feat: block can-be-merged when unresolved review conversations exist#998
Conversation
Use GitHub GraphQL API to check for unresolved review threads when branch-protection.required_conversation_resolution is enabled. Includes cursor-based pagination, comment URLs in failure output, and outdated thread markers. Closes #997
Collaborator
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
Review Summary by QodoBlock PR merge when unresolved review conversations exist
WalkthroughsDescription• Add unresolved review conversation check to can-be-merged workflow • Implement GitHub GraphQL API with cursor-based pagination for threads • Display clickable URLs and outdated markers in check output • Read configuration from existing branch-protection settings Diagramflowchart LR
Config["branch-protection config<br/>required_conversation_resolution"]
GraphQL["GitHub GraphQL API<br/>reviewThreads query"]
Pagination["Cursor-based pagination<br/>fetch all threads"]
Filter["Filter unresolved threads<br/>include outdated marker"]
Output["Check run output<br/>with thread URLs"]
Merge["can-be-merged check<br/>pass/fail decision"]
Config --> GraphQL
GraphQL --> Pagination
Pagination --> Filter
Filter --> Output
Output --> Merge
File Changes1. webhook_server/libs/github_api.py
|
Code Review by Qodo
1.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds branch-protection-backed conversation-resolution: new public Changes
Sequence DiagramsequenceDiagram
participant Handler as Pull Request Handler
participant GH as GithubWebhook
participant Checks as Check Runs/Statuses Service
participant API as GitHub GraphQL API
Handler->>Handler: check_if_can_be_merged(pr)
Handler->>GH: read required_conversation_resolution
alt enabled
par Parallel fetch
Handler->>Checks: fetch last commit check runs
Handler->>Checks: fetch commit statuses
Handler->>GH: get_unresolved_review_threads(pr_number)
GH->>API: GraphQL query (paginated) for reviewThreads
API-->>GH: thread pages (path,line,isOutdated,url)
GH-->>Handler: unresolved threads list
and
end
Handler->>Handler: evaluate unresolved threads
alt threads found
Handler->>Handler: build conversation_failure block with thread details
Handler->>Handler: append to failure_output (merge blocked)
else no threads
Handler->>Handler: continue other merge checks
end
else disabled
par Parallel fetch
Handler->>Checks: fetch last commit check runs
Handler->>Checks: fetch commit statuses
end
Handler->>Handler: continue other merge checks
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webhook_server/tests/test_pull_request_handler.py`:
- Around line 1095-1358: Several tests assign the literal "test-token" to
pull_request_handler.github_webhook.token; replace those hardcoded tokens with
the shared TEST_GITHUB_TOKEN constant to satisfy secret-scanning and project
conventions. Update every occurrence where the test sets
pull_request_handler.github_webhook.token = "test-token" (within tests calling
GithubWebhook.get_unresolved_review_threads) to use TEST_GITHUB_TOKEN instead,
and ensure the test module imports or references the TEST_GITHUB_TOKEN constant
available in the test suite.
- Around line 809-821: Replace positional boolean arguments in patch.object
calls with the explicit new= keyword to silence Ruff FBT003 and clarify intent;
for example change patch.object(mock_pull_request, "mergeable", True) to
patch.object(mock_pull_request, "mergeable", new=True) and
patch.object(pull_request_handler.github_webhook,
"required_conversation_resolution", False) to
patch.object(pull_request_handler.github_webhook,
"required_conversation_resolution", new=False); apply the same change for all
occurrences in this test file (e.g., around the mock_pull_request patches,
patches on pull_request_handler.github_webhook, and any other patch.object calls
that currently pass booleans positionally).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
webhook_server/libs/github_api.pywebhook_server/libs/handlers/pull_request_handler.pywebhook_server/tests/test_pull_request_handler.pywebhook_server/tests/test_schema_validator.py
- Validate required_conversation_resolution as bool via bool() cast - Use DEFAULT_BRANCH_PROTECTION default (True) instead of hardcoded False - Use TEST_GITHUB_TOKEN constant instead of hardcoded test tokens - Use new= keyword for boolean patch.object calls (Ruff FBT003)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webhook_server/libs/github_api.py`:
- Around line 933-938: The code assumes data["data"]["repository"] is present
but GitHub can return repository: null; update the block that assigns pr_data
(currently: pr_data = data["data"]["repository"]["pullRequest"]) to first check
repository = data["data"].get("repository") and if repository is None raise a
ValueError with a clear message including self.repository_full_name and
pr_number to indicate missing repository or insufficient token access, then
proceed to fetch pullRequest and reviewThreads as before (refer to symbols
pr_data, review_threads, repository_full_name, pr_number).
- Around line 738-755: The code currently uses bool(...) which treats strings
like "false" as True; change the assignment logic around
required_conversation_resolution to perform explicit type validation: when
reading _repo_bp[_bp_key] or _global_bp[_bp_key], check isinstance(value, bool)
and only accept it if True; if the value is not a bool, log a warning (use the
instance logger, e.g., self.logger.warning) indicating invalid branch-protection
value for required_conversation_resolution and fall back to _bp_default (or the
next valid scope), rather than casting; ensure this validation is applied to
both repository-level and global-level reads so that DEFAULT_BRANCH_PROTECTION
remains the safe default.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
webhook_server/libs/github_api.pywebhook_server/tests/test_pull_request_handler.py
- Use isinstance(value, bool) instead of bool() cast to prevent "false" string being treated as True - Log warning on invalid config values and fall back to default - Handle repository: null in GraphQL response with clear ValueError - Add test for null repository response
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webhook_server/libs/github_api.py`:
- Line 741: Replace the module-level assert that checks _bp_raw_default with a
runtime check that always runs: in the code around symbol _bp_raw_default (used
when building DEFAULT_BRANCH_PROTECTION), change the assert
isinstance(_bp_raw_default, bool) into an explicit conditional that raises a
TypeError (or ValueError) with a clear message like
"DEFAULT_BRANCH_PROTECTION[<key>] must be bool" when the check fails, so the
invariant holds even when Python runs with -O; ensure the error message
references _bp_key/_bp_raw_default to aid debugging.
- Around line 738-761: The warning uses self.log_prefix which may not be set
when _repo_data_from_config is first called; change the log prefix access to use
getattr(self, "log_prefix", "") in the branch-protection validation block so the
warning won't raise if log_prefix is missing. Specifically update the code
around required_conversation_resolution assignment and the logger.warning call
inside the for loop in _repo_data_from_config to use getattr(self, "log_prefix",
"") (same pattern as used later at lines with other warnings) so misconfigured
branch-protection values emit a warning instead of crashing.
In `@webhook_server/tests/test_pull_request_handler.py`:
- Around line 804-1033: The four tests
test_can_be_merged_conversation_resolution_disabled,
test_can_be_merged_no_unresolved_threads,
test_can_be_merged_unresolved_threads_present, and
test_can_be_merged_multiple_unresolved_threads duplicate the same large patch
block; extract that repeated patch setup into a single helper (either a
`@contextmanager` or a pytest fixture) that applies the common patch.object calls
against PullRequestHandler instance (including patches for
_check_if_pr_approved, _check_labels_for_can_be_merged,
labels_handler._add_label/_remove_label,
owners_file_handler.owners_data_for_changed_files, github_webhook.minimum_lgtm,
check_run_handler.set_check_in_progress, required_check_in_progress,
required_check_failed_or_no_status, labels_handler.wip_or_hold_labels_exists,
labels_handler.pull_request_labels_names, github_webhook.last_commit) and accept
parameters for the varying pieces
(github_webhook.required_conversation_resolution,
github_webhook.get_unresolved_review_threads return value, and which label mock
to expose); then update each test to call the helper and use the returned mocks
(e.g., mock_add_label, mock_remove_label, mock_get_threads,
mock_set_check_failure) so each test only specifies the small variations and
assertions.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
webhook_server/libs/github_api.pywebhook_server/tests/test_pull_request_handler.py
- Use getattr(self, "log_prefix", "") to prevent AttributeError when _repo_data_from_config runs before log_prefix is set - Replace assert isinstance() with explicit TypeError raise to ensure validation is not stripped by python -O
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webhook_server/libs/github_api.py`:
- Around line 760-764: The warning logs an incorrect fallback value
(_bp_default) when a repo-level value (_bp_val) is invalid but a valid
global/current value is already applied; update the logger.warning call (the
call using self.logger.warning and log_prefix) to include the actual
effective/current value that will be used instead of _bp_default (read whichever
variable holds the currently applied value before override, e.g., the config
field or local "current" variable) so the message reports the true fallback
rather than the literal _bp_default.
- Around line 969-974: Replace the current warning-and-break behavior when
page_info["endCursor"] is null with a fail-fast exception: instead of logging
via self.logger.warning(...) and breaking, raise a ValueError (or KeyError) that
includes context (self.log_prefix, pr_number and page_info) so callers cannot
receive partial results; update the code around variables cursor and page_info
to throw the exception when hasNextPage is True but endCursor is None, ensuring
the error propagates to the caller.
- Warning message now shows kept current value instead of hardcoded default - Null cursor with hasNextPage=True now raises ValueError instead of returning partial data that could allow incorrect merge approval
Reduce ~80 lines of duplication across 4 can-be-merged conversation resolution tests by extracting shared patch.object setup into a _can_be_merged_patch_context contextmanager helper.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
can-be-mergedworkflowrequired_conversation_resolutionfrom existingbranch-protectionconfig (no new config options)(outdated)markerChanges
webhook_server/libs/github_api.py—get_unresolved_review_threads()GraphQL method with paginationwebhook_server/libs/handlers/pull_request_handler.py— integration incheck_if_can_be_merged()webhook_server/tests/test_pull_request_handler.py— 11 new test casesTest plan
Summary by CodeRabbit