diff --git a/gittensor/utils/github_api_tools.py b/gittensor/utils/github_api_tools.py index 19e95167..d01a517f 100644 --- a/gittensor/utils/github_api_tools.py +++ b/gittensor/utils/github_api_tools.py @@ -165,7 +165,7 @@ def get_github_identity(token: str) -> GitHubIdentityResult: return GitHubIdentityResult(None, GitHubIdentityStatus.TRANSIENT_FAILURE) -# GraphQL fragment used by both issue submissions and solver detection. +# GraphQL fragment used by issue submissions / PR discovery. _PR_TIMELINE_QUERY = """ query($owner: String!, $name: String!, $issueNumber: Int!) { repository(owner: $owner, name: $name) { @@ -202,6 +202,36 @@ def get_github_identity(token: str) -> GitHubIdentityResult: """ +_ISSUE_CLOSURE_QUERY = """ +query($owner: String!, $name: String!, $issueNumber: Int!) { + repository(owner: $owner, name: $name) { + issue(number: $issueNumber) { + closedAt + timelineItems(itemTypes: [CLOSED_EVENT], last: 20) { + nodes { + ... on ClosedEvent { + createdAt + stateReason + closer { + __typename + ... on PullRequest { + number + state + merged + mergedAt + author { ... on User { databaseId login } } + baseRepository { nameWithOwner } + } + } + } + } + } + } + } +} +""" + + def _resolve_pr_state(raw_state: str, merged: bool = False) -> str: """Normalize PR state to uppercase GraphQL-style values.""" if merged: @@ -379,51 +409,117 @@ def execute_graphql_query( return None -def find_solver_from_cross_references( +def _is_completed_close_event(node: Dict[str, Any]) -> bool: + state_reason = node.get('stateReason') + return state_reason is None or str(state_reason).strip().upper() == 'COMPLETED' + + +def _select_current_close_event(issue_data: Dict[str, Any]) -> Optional[Dict[str, Any]]: + """Return the ClosedEvent that represents the issue's current closure.""" + timeline_nodes = issue_data.get('timelineItems', {}).get('nodes', []) or [] + closed_at = issue_data.get('closedAt') + if not closed_at: + return None + + completed_events = [node for node in timeline_nodes if node and _is_completed_close_event(node)] + if not completed_events: + return None + + for node in reversed(completed_events): + if node.get('createdAt') == closed_at: + return node + return None + + +def _solver_from_closed_event(repo: str, event: Dict[str, Any]) -> tuple[Optional[int], Optional[int]]: + target_repo = repo.lower() + closer = event.get('closer') or {} + if closer.get('__typename') != 'PullRequest': + return None, None + + base_repo = ((closer.get('baseRepository') or {}).get('nameWithOwner') or '').lower() + if base_repo != target_repo: + bt.logging.warning( + f'ClosedEvent closer PR#{closer.get("number")} targets {base_repo or "unknown repo"}, not {target_repo}' + ) + return None, None + + state = _resolve_pr_state(closer.get('state', ''), merged=bool(closer.get('merged', False))) + if state != 'MERGED': + bt.logging.warning(f'ClosedEvent closer PR#{closer.get("number")} is not merged (state={state})') + return None, None + + author = closer.get('author') or {} + return author.get('databaseId'), closer.get('number') + + +def find_solver_from_closure_event( repo: str, issue_number: int, token: str ) -> Optional[tuple[Optional[int], Optional[int]]]: - """Resolve solver from cross-referenced PRs on the issue timeline. - - This uses ``_search_issue_referencing_prs_graphql`` and then narrows to PRs - that are: - - merged, and - - explicitly closing ``issue_number``. + """Resolve the issue solver from GitHub's authoritative current close event. - If multiple candidates exist, the earliest ``merged_at`` is selected, since - GitHub closes an issue on the first merged PR that triggers the close; later - PRs declaring "Closes #X" in their body still appear in the timeline with - ``closingIssuesReferences`` populated even though they did not actually - close the issue. PR ``number`` is used as a deterministic tiebreaker. + Cross-reference and ``closingIssuesReferences`` entries are declarations made + by PR text, not proof that a PR caused the issue's current closure. Bounty + attribution must therefore read the ``ClosedEvent.closer`` for the issue's + current ``closedAt`` value and only accept a merged PR targeting the same repo. Returns: ``None`` when lookup fails and should be retried later. Otherwise a tuple ``(solver_github_id, pr_number)`` where either value may be ``None`` when no valid closing PR is found. """ - prs = _search_issue_referencing_prs_graphql(repo, issue_number, token, open_only=False) - if prs is None: + if not token: + return None, None + if issue_number < 1 or '/' not in repo: + return None, None + owner, name = repo.split('/', 1) + owner = owner.strip() + name = name.strip() + if not owner or not name: + return None, None + + result = execute_graphql_query( + query=_ISSUE_CLOSURE_QUERY, + variables={'owner': owner, 'name': name, 'issueNumber': issue_number}, + token=token, + max_attempts=3, + ) + if result is None: + bt.logging.warning(f'GraphQL closure query failed for {repo}#{issue_number}') return None - merged = [p for p in prs if p.get('state') == 'MERGED' and issue_number in p.get('closing_numbers', [])] - bt.logging.debug(f'Found {len(merged)} verified closing PRs via GraphQL for {repo}#{issue_number}') - if not merged: - return None, None + errors = result.get('errors') + if errors: + bt.logging.warning(f'GraphQL closure query returned errors for {repo}#{issue_number}: {errors}') + return None - if len(merged) > 1: - bt.logging.warning(f'Multiple closing PRs found for {repo}#{issue_number}, selecting earliest-merged.') - for candidate in merged: - bt.logging.debug( - f' PR#{candidate.get("number")}, solver_id={candidate.get("author_id")}, ' - f'merged_at={candidate.get("merged_at")}' - ) + issue_data = result.get('data', {}).get('repository', {}).get('issue') + if issue_data is None: + bt.logging.warning(f'GraphQL closure response missing issue data for {repo}#{issue_number}') + return None - merged.sort(key=lambda p: (p.get('merged_at') or '', p.get('number') or 0)) - best = merged[0] + close_event = _select_current_close_event(issue_data) + if close_event is None: + return None, None + + solver_github_id, pr_number = _solver_from_closed_event(f'{owner}/{name}', close_event) bt.logging.debug( - f'Solver via GraphQL cross-reference: PR#{best.get("number")}, ' - f'solver_id={best.get("author_id")}, merged_at={best.get("merged_at")}' + f'Solver via GraphQL close event: PR#{pr_number}, ' + f'solver_id={solver_github_id}, closed_at={close_event.get("createdAt")}' ) - return best.get('author_id'), best.get('number') + return solver_github_id, pr_number + + +def find_solver_from_cross_references( + repo: str, issue_number: int, token: str +) -> Optional[tuple[Optional[int], Optional[int]]]: + """Compatibility wrapper for bounty solver lookup. + + Solver attribution used to infer the closer from cross-referenced PRs. The + implementation now uses ``ClosedEvent.closer`` to avoid rewarding stale or + attacker-influenceable cross-reference candidates. + """ + return find_solver_from_closure_event(repo, issue_number, token) def check_github_issue_closed(repo: str, issue_number: int, token: str) -> Optional[Dict[str, Any]]: @@ -467,7 +563,7 @@ def check_github_issue_closed(repo: str, issue_number: int, token: str) -> Optio } bt.logging.debug(f'Finding solver for {repo}#{issue_number}') - solver_lookup = find_solver_from_cross_references(repo, issue_number, token) + solver_lookup = find_solver_from_closure_event(repo, issue_number, token) if solver_lookup is None: bt.logging.warning(f'Solver lookup failed for {repo}#{issue_number}') solver_lookup_failed = True diff --git a/tests/utils/test_github_api_tools.py b/tests/utils/test_github_api_tools.py index 4a55088b..c3d4d92b 100644 --- a/tests/utils/test_github_api_tools.py +++ b/tests/utils/test_github_api_tools.py @@ -228,16 +228,69 @@ def _pr_node( } +def _closure_graphql_response(nodes, closed_at=None): + """Helper to build a GraphQL closed-event response.""" + if closed_at is None and nodes: + closed_at = nodes[-1].get('createdAt') + return { + 'data': { + 'repository': { + 'issue': { + 'closedAt': closed_at, + 'timelineItems': { + 'nodes': nodes, + }, + }, + }, + }, + } + + +def _closed_event_pr_node( + number, + merged=True, + merged_at='2025-06-01T00:00:00Z', + user_id=42, + base_repo='owner/repo', + state_reason='COMPLETED', + created_at='2025-06-01T00:00:01Z', +): + """Helper to build a single closed-event node whose closer is a PR.""" + return { + 'createdAt': created_at, + 'stateReason': state_reason, + 'closer': { + '__typename': 'PullRequest', + 'number': number, + 'state': 'MERGED' if merged else 'OPEN', + 'merged': merged, + 'mergedAt': merged_at, + 'author': {'databaseId': user_id}, + 'baseRepository': {'nameWithOwner': base_repo}, + }, + } + + +def _closed_event_non_pr_node(typename='Commit', state_reason='COMPLETED', created_at='2025-06-01T00:00:01Z'): + return { + 'createdAt': created_at, + 'stateReason': state_reason, + 'closer': { + '__typename': typename, + }, + } + + class TestFindSolverFromCrossReferences: - """Test suite for find_solver_from_cross_references (GraphQL-based solver detection).""" + """Test suite for solver detection through GitHub's authoritative close event.""" @patch('gittensor.utils.github_api_tools.execute_graphql_query') @patch('gittensor.utils.github_api_tools.bt.logging') def test_single_merged_pr_closing_issue(self, mock_logging, mock_graphql): - """Single merged PR with closing reference returns correct solver.""" - mock_graphql.return_value = _graphql_response( + """Single merged PR closer returns correct solver.""" + mock_graphql.return_value = _closure_graphql_response( [ - _pr_node(number=14, user_id=42, closing_issues=[12]), + _closed_event_pr_node(number=14, user_id=42), ] ) @@ -245,14 +298,17 @@ def test_single_merged_pr_closing_issue(self, mock_logging, mock_graphql): assert solver_id == 42 assert pr_number == 14 + query = mock_graphql.call_args.kwargs['query'] + assert 'CLOSED_EVENT' in query + assert 'CROSS_REFERENCED_EVENT' not in query @patch('gittensor.utils.github_api_tools.execute_graphql_query') @patch('gittensor.utils.github_api_tools.bt.logging') def test_unmerged_pr_is_filtered_out(self, mock_logging, mock_graphql): - """Unmerged PRs are ignored even if they have closing references.""" - mock_graphql.return_value = _graphql_response( + """Unmerged PR closers are ignored.""" + mock_graphql.return_value = _closure_graphql_response( [ - _pr_node(number=14, merged=False, user_id=42, closing_issues=[12]), + _closed_event_pr_node(number=14, merged=False, user_id=42), ] ) @@ -264,10 +320,10 @@ def test_unmerged_pr_is_filtered_out(self, mock_logging, mock_graphql): @patch('gittensor.utils.github_api_tools.execute_graphql_query') @patch('gittensor.utils.github_api_tools.bt.logging') def test_pr_from_different_repo_is_filtered_out(self, mock_logging, mock_graphql): - """PRs targeting a different base repo are rejected (prevents cross-repo gaming).""" - mock_graphql.return_value = _graphql_response( + """PR closers targeting a different base repo are rejected.""" + mock_graphql.return_value = _closure_graphql_response( [ - _pr_node(number=14, user_id=99, base_repo='attacker/evil-repo', closing_issues=[12]), + _closed_event_pr_node(number=14, user_id=99, base_repo='attacker/evil-repo'), ] ) @@ -279,10 +335,10 @@ def test_pr_from_different_repo_is_filtered_out(self, mock_logging, mock_graphql @patch('gittensor.utils.github_api_tools.execute_graphql_query') @patch('gittensor.utils.github_api_tools.bt.logging') def test_pr_mentioning_but_not_closing_issue_is_filtered_out(self, mock_logging, mock_graphql): - """PRs that mention the issue but don't have it in closingIssuesReferences are ignored.""" - mock_graphql.return_value = _graphql_response( + """Non-PR closers are not attributed to a miner.""" + mock_graphql.return_value = _closure_graphql_response( [ - _pr_node(number=14, user_id=42, closing_issues=[99]), # Closes #99, not #12 + _closed_event_non_pr_node('Commit'), ] ) @@ -294,10 +350,10 @@ def test_pr_mentioning_but_not_closing_issue_is_filtered_out(self, mock_logging, @patch('gittensor.utils.github_api_tools.execute_graphql_query') @patch('gittensor.utils.github_api_tools.bt.logging') def test_cross_repo_closing_issue_number_collision_is_filtered_out(self, mock_logging, mock_graphql): - """A PR closing another repo's same-numbered issue must not solve this bounty issue.""" - mock_graphql.return_value = _graphql_response( + """Project-driven closures are not attributed to a miner.""" + mock_graphql.return_value = _closure_graphql_response( [ - _pr_node(number=14, user_id=42, closing_issues=[12], closing_repo='other/repo'), + _closed_event_non_pr_node('ProjectV2'), ] ) @@ -309,10 +365,10 @@ def test_cross_repo_closing_issue_number_collision_is_filtered_out(self, mock_lo @patch('gittensor.utils.github_api_tools.execute_graphql_query') @patch('gittensor.utils.github_api_tools.bt.logging') def test_closing_issue_repo_check_is_case_insensitive(self, mock_logging, mock_graphql): - """GitHub repository names are case-insensitive when validating closing refs.""" - mock_graphql.return_value = _graphql_response( + """GitHub repository names are case-insensitive when validating closer PRs.""" + mock_graphql.return_value = _closure_graphql_response( [ - _pr_node(number=14, user_id=42, closing_issues=[12], closing_repo='Owner/Repo'), + _closed_event_pr_node(number=14, user_id=42, base_repo='Owner/Repo'), ] ) @@ -323,58 +379,63 @@ def test_closing_issue_repo_check_is_case_insensitive(self, mock_logging, mock_g @patch('gittensor.utils.github_api_tools.execute_graphql_query') @patch('gittensor.utils.github_api_tools.bt.logging') - def test_multiple_candidates_picks_earliest_merged(self, mock_logging, mock_graphql): - """When multiple merged PRs declare the same closing reference, the earliest-merged one - is selected — GitHub closes the issue on the first merge; later PRs that put - 'Closes #X' in their body still appear in closingIssuesReferences but did not - actually close the issue, so they must not capture solver attribution.""" - mock_graphql.return_value = _graphql_response( + def test_current_close_event_wins_over_stale_close_event(self, mock_logging, mock_graphql): + """The current close event wins over stale close events from earlier cycles.""" + mock_graphql.return_value = _closure_graphql_response( [ - _pr_node(number=10, user_id=100, merged_at='2025-01-01T00:00:00Z', closing_issues=[12]), - _pr_node(number=20, user_id=200, merged_at='2025-06-15T00:00:00Z', closing_issues=[12]), - _pr_node(number=15, user_id=150, merged_at='2025-03-01T00:00:00Z', closing_issues=[12]), - ] + _closed_event_pr_node( + number=10, + user_id=100, + merged_at='2025-01-01T00:00:00Z', + created_at='2025-01-01T00:00:01Z', + ), + _closed_event_pr_node( + number=20, + user_id=200, + merged_at='2025-06-15T00:00:00Z', + created_at='2025-06-15T00:00:01Z', + ), + ], + closed_at='2025-06-15T00:00:01Z', ) solver_id, pr_number = find_solver_from_cross_references('owner/repo', 12, 'fake_token') - assert solver_id == 100 - assert pr_number == 10 - mock_logging.warning.assert_called() # Should warn about multiple candidates + assert solver_id == 200 + assert pr_number == 20 @patch('gittensor.utils.github_api_tools.execute_graphql_query') @patch('gittensor.utils.github_api_tools.bt.logging') def test_mixed_valid_and_invalid_candidates(self, mock_logging, mock_graphql): - """Only valid candidates survive all filters (merged + same repo + closing ref).""" - mock_graphql.return_value = _graphql_response( + """An invalid current closure does not fall back to an older valid PR closure.""" + mock_graphql.return_value = _closure_graphql_response( [ - # Invalid: unmerged - _pr_node(number=10, merged=False, user_id=100, closing_issues=[12]), - # Invalid: wrong repo - _pr_node(number=11, user_id=101, base_repo='other/repo', closing_issues=[12]), - # Invalid: doesn't close this issue - _pr_node(number=13, user_id=103, closing_issues=[99]), - # Valid - _pr_node(number=14, user_id=42, merged_at='2025-06-01T00:00:00Z', closing_issues=[12]), - ] + _closed_event_pr_node( + number=14, + user_id=42, + merged_at='2025-01-01T00:00:00Z', + created_at='2025-01-01T00:00:01Z', + ), + _closed_event_non_pr_node('Commit', created_at='2025-06-01T00:00:01Z'), + ], + closed_at='2025-06-01T00:00:01Z', ) solver_id, pr_number = find_solver_from_cross_references('owner/repo', 12, 'fake_token') - assert solver_id == 42 - assert pr_number == 14 + assert solver_id is None + assert pr_number is None @patch('gittensor.utils.github_api_tools.execute_graphql_query') @patch('gittensor.utils.github_api_tools.bt.logging') def test_fork_pr_targeting_main_repo_is_accepted(self, mock_logging, mock_graphql): - """PRs from forks that target the main repo (baseRepository matches) are accepted.""" - mock_graphql.return_value = _graphql_response( + """Fork PRs that target the main repo (baseRepository matches) are accepted.""" + mock_graphql.return_value = _closure_graphql_response( [ - _pr_node( + _closed_event_pr_node( number=14, user_id=42, base_repo='owner/repo', # PR targets the main repo - closing_issues=[12], ), ] ) @@ -388,9 +449,9 @@ def test_fork_pr_targeting_main_repo_is_accepted(self, mock_logging, mock_graphq @patch('gittensor.utils.github_api_tools.bt.logging') def test_base_repo_check_is_case_insensitive(self, mock_logging, mock_graphql): """Base repo comparison is case-insensitive (GitHub repos are case-insensitive).""" - mock_graphql.return_value = _graphql_response( + mock_graphql.return_value = _closure_graphql_response( [ - _pr_node(number=14, user_id=42, base_repo='Owner/Repo', closing_issues=[12]), + _closed_event_pr_node(number=14, user_id=42, base_repo='Owner/Repo'), ] ) @@ -401,9 +462,40 @@ def test_base_repo_check_is_case_insensitive(self, mock_logging, mock_graphql): @patch('gittensor.utils.github_api_tools.execute_graphql_query') @patch('gittensor.utils.github_api_tools.bt.logging') - def test_no_cross_references_returns_none(self, mock_logging, mock_graphql): + def test_no_close_events_returns_none(self, mock_logging, mock_graphql): """Empty timeline nodes returns (None, None).""" - mock_graphql.return_value = _graphql_response([]) + mock_graphql.return_value = _closure_graphql_response([]) + + solver_id, pr_number = find_solver_from_cross_references('owner/repo', 12, 'fake_token') + + assert solver_id is None + assert pr_number is None + + @patch('gittensor.utils.github_api_tools.execute_graphql_query') + @patch('gittensor.utils.github_api_tools.bt.logging') + def test_missing_closed_at_does_not_use_stale_close_event(self, mock_logging, mock_graphql): + """Without closedAt, an older close event is not authoritative enough to attribute.""" + mock_graphql.return_value = _closure_graphql_response( + [ + _closed_event_pr_node(number=14, user_id=42), + ], + closed_at='', + ) + + solver_id, pr_number = find_solver_from_cross_references('owner/repo', 12, 'fake_token') + + assert solver_id is None + assert pr_number is None + + @patch('gittensor.utils.github_api_tools.execute_graphql_query') + @patch('gittensor.utils.github_api_tools.bt.logging') + def test_non_completed_close_event_does_not_attribute_solver(self, mock_logging, mock_graphql): + """Non-completed close events never produce a bounty solver.""" + mock_graphql.return_value = _closure_graphql_response( + [ + _closed_event_pr_node(number=14, user_id=42, state_reason='NOT_PLANNED'), + ] + ) solver_id, pr_number = find_solver_from_cross_references('owner/repo', 12, 'fake_token') @@ -442,6 +534,29 @@ def test_graphql_failure_sets_solver_lookup_failed(self, mock_logging, mock_get, 'solver_lookup_failed': True, } + @patch('gittensor.utils.github_api_tools.execute_graphql_query') + @patch('gittensor.utils.github_api_tools.requests.get') + @patch('gittensor.utils.github_api_tools.bt.logging') + def test_completed_issue_uses_closed_event_closer(self, mock_logging, mock_get, mock_graphql): + issue_response = Mock() + issue_response.status_code = 200 + issue_response.json.return_value = {'state': 'closed', 'state_reason': 'completed'} + mock_get.return_value = issue_response + mock_graphql.return_value = _closure_graphql_response( + [ + _closed_event_pr_node(number=900, user_id=999), + ] + ) + + result = check_github_issue_closed('owner/repo', 12, 'fake_token') + + assert result == { + 'is_closed': True, + 'solver_github_id': 999, + 'pr_number': 900, + 'solver_lookup_failed': False, + } + @patch('gittensor.utils.github_api_tools.execute_graphql_query') @patch('gittensor.utils.github_api_tools.requests.get') @patch('gittensor.utils.github_api_tools.bt.logging')