diff --git a/gittensor/classes.py b/gittensor/classes.py index b9168f494..4ab0b4bf3 100644 --- a/gittensor/classes.py +++ b/gittensor/classes.py @@ -392,6 +392,7 @@ class MinerEvaluation: open_pull_requests: List[PullRequest] = field(default_factory=list) closed_pull_requests: List[PullRequest] = field(default_factory=list) stale_closed_pull_requests: List[PullRequest] = field(default_factory=list) + stale_merged_pull_requests: List[PullRequest] = field(default_factory=list) # Populated by gittensor.validator.oss_contributions.mirror.combine.combine # when the mirror scoring path runs. Empty for legacy-only evaluations. @@ -510,6 +511,13 @@ def add_stale_closed_pull_request(self, raw_pr: Dict): PullRequest.from_graphql_response(raw_pr, self.uid, self.hotkey, self.github_id) ) + def add_stale_merged_pull_request(self, raw_pr: Dict): + """Track a stale MERGED PR so storage can refresh its pull_requests row.""" + bt.logging.info(f'Stale MERGED PR #{raw_pr["number"]} in {parse_repo_name(raw_pr["repository"])}') + self.stale_merged_pull_requests.append( + PullRequest.from_graphql_response(raw_pr, self.uid, self.hotkey, self.github_id) + ) + @dataclass class ScoreBreakdown: diff --git a/gittensor/utils/github_api_tools.py b/gittensor/utils/github_api_tools.py index 74a14143e..fa10df89d 100644 --- a/gittensor/utils/github_api_tools.py +++ b/gittensor/utils/github_api_tools.py @@ -827,6 +827,21 @@ def try_add_open_or_closed_pr( miner_eval.add_closed_pull_request(pr_raw) +def _maybe_add_stale_merged_pr( + miner_eval: MinerEvaluation, + pr_raw: Dict, + lookback_date_filter: datetime, +) -> None: + """Capture merged PRs whose mergedAt predates the lookback window so storage + can sync the previously-stored OPEN row (parallel to #769's stale-CLOSED path). + """ + merged_at = pr_raw.get('mergedAt') + if not merged_at: + return + if parse_github_iso_to_utc(merged_at) < lookback_date_filter: + miner_eval.add_stale_merged_pull_request(pr_raw) + + def should_skip_merged_pr( pr_raw: Dict, repository_full_name: str, @@ -1014,6 +1029,7 @@ def load_miners_prs( ) if should_skip: + _maybe_add_stale_merged_pr(miner_eval, pr_raw, lookback_date_filter) bt.logging.debug(skip_reason or '') continue diff --git a/gittensor/validator/storage/queries.py b/gittensor/validator/storage/queries.py index 308a26d43..7e26a60ee 100644 --- a/gittensor/validator/storage/queries.py +++ b/gittensor/validator/storage/queries.py @@ -100,6 +100,22 @@ updated_at = NOW() """ +# Targeted UPDATE that syncs pr_state to MERGED on existing rows whose PR +# merged outside the lookback window. The pr_state guard keeps it idempotent +# and preserves historical scoring fields on rows already captured as MERGED. +REFRESH_STALE_MERGED_PR_STATE = """ +UPDATE pull_requests +SET + pr_state = 'MERGED', + merged_at = %s, + collateral_score = 0.0, + updated_at = NOW() +WHERE + number = %s + AND repository_full_name = %s + AND pr_state != 'MERGED' +""" + # Targeted state-only refresh for stale-closed PRs (avoids overwriting scored columns) REFRESH_STALE_PR_STATES = """ UPDATE pull_requests diff --git a/gittensor/validator/storage/repository.py b/gittensor/validator/storage/repository.py index 4584b69c9..23263664e 100644 --- a/gittensor/validator/storage/repository.py +++ b/gittensor/validator/storage/repository.py @@ -23,6 +23,7 @@ CLEANUP_STALE_MINER_EVALUATIONS_BY_HOTKEY, CLEANUP_STALE_MINERS, CLEANUP_STALE_MINERS_BY_HOTKEY, + REFRESH_STALE_MERGED_PR_STATE, REFRESH_STALE_PR_STATES, SET_MINER, ) @@ -218,6 +219,30 @@ def store_pull_requests_bulk(self, pull_requests: List[PullRequest], commit: boo self.logger.error(f'Error in bulk pull request storage: {e}') return 0 + def refresh_stale_merged_pr_states(self, pull_requests: List[PullRequest], commit: bool = True) -> int: + """Targeted UPDATE-only refresh of pr_state/merged_at on existing rows. + + Skips rows already at pr_state='MERGED' so historical scoring fields are + preserved; rows that don't exist are silent no-ops (never inserts). + """ + eligible = [pr for pr in pull_requests if pr.merged_at is not None] + if not eligible: + return 0 + + values = [(pr.merged_at, pr.number, pr.repository_full_name) for pr in eligible] + + try: + with self.get_cursor() as cursor: + cursor.executemany(REFRESH_STALE_MERGED_PR_STATE, values) + if commit: + self.db.commit() + return len(values) + except Exception as e: + if commit: + self.db.rollback() + self.logger.error(f'Error refreshing stale merged PR states: {e}') + return 0 + def refresh_stale_pr_states(self, pull_requests: List[PullRequest], commit: bool = True) -> int: """Update pr_state to CLOSED for stale PRs without touching scoring columns. diff --git a/gittensor/validator/utils/storage.py b/gittensor/validator/utils/storage.py index 97e838592..9adbdaf9f 100644 --- a/gittensor/validator/utils/storage.py +++ b/gittensor/validator/utils/storage.py @@ -74,6 +74,9 @@ def _adapt_mirror(scored_list): result.stored_counts['stale_closed_pull_requests'] = self.repo.refresh_stale_pr_states( miner_eval.stale_closed_pull_requests, commit=False ) + result.stored_counts['stale_merged_pull_requests'] = self.repo.refresh_stale_merged_pr_states( + miner_eval.stale_merged_pull_requests, commit=False + ) result.stored_counts['issues'] = self.repo.store_issues_bulk(miner_eval.get_all_issues(), commit=False) result.stored_counts['file_changes'] = self.repo.store_file_changes_bulk( miner_eval.get_all_file_changes(), commit=False diff --git a/tests/validator/test_unscored_stale_prs.py b/tests/validator/test_unscored_stale_prs.py index a72f7a00d..b62c11bb2 100644 --- a/tests/validator/test_unscored_stale_prs.py +++ b/tests/validator/test_unscored_stale_prs.py @@ -1,11 +1,19 @@ # The MIT License (MIT) # Copyright © 2025 Entrius -"""Regression tests for stale-closed PR capture in a storage-only bucket. +"""Regression tests for stale-closed and stale-merged PR capture in storage-only buckets. -Covers the invariant that stale-CLOSED PRs (created before the lookback window): -- do not enter `closed_pull_requests` (preserves the #406 drop-from-scoring) -- do enter `stale_closed_pull_requests` so storage can refresh pr_state +Covers two invariants: + +stale-CLOSED PRs (created before the lookback window, #769): +- do not enter ``closed_pull_requests`` (preserves the #406 drop-from-scoring) +- do enter ``stale_closed_pull_requests`` so storage can refresh ``pr_state`` +- are not reflected in total counts or scoring buckets + +stale-MERGED PRs (merged before the lookback window): +- do not enter ``merged_pull_requests`` (preserves the lookback exclusion) +- do enter ``stale_merged_pull_requests`` so storage can sync a previously- + stored OPEN row to MERGED without re-entering scoring - are not reflected in total counts or scoring buckets """ @@ -13,7 +21,7 @@ from unittest.mock import patch from gittensor.classes import MinerEvaluation, PRState -from gittensor.utils.github_api_tools import try_add_open_or_closed_pr +from gittensor.utils.github_api_tools import _maybe_add_stale_merged_pr, try_add_open_or_closed_pr def _pr_node(number: int, created_at: str, closed_at: str, state: str = 'CLOSED') -> dict: @@ -130,3 +138,123 @@ def test_stale_closed_storage_bucket_does_not_inflate_any_totals(_): assert miner_eval.total_closed_prs == 0 assert miner_eval.total_prs == 0 assert len(miner_eval.stale_closed_pull_requests) == 3 + + +# --------------------------------------------------------------------------- +# Stale-MERGED PR capture (storage-only refresh path) +# --------------------------------------------------------------------------- + + +def _merged_pr_node(number: int, created_at: str, merged_at: str) -> dict: + return { + 'number': number, + 'title': f'merged PR {number}', + 'state': 'MERGED', + 'repository': { + 'name': 'jan', + 'owner': {'login': 'janhq'}, + 'defaultBranchRef': {'name': 'main'}, + }, + 'headRepository': {'name': 'jan', 'owner': {'login': 'contributor'}}, + 'author': {'login': 'contributor'}, + 'authorAssociation': 'CONTRIBUTOR', + 'mergedBy': {'login': 'maintainer'}, + 'mergedAt': merged_at, + 'createdAt': created_at, + 'closedAt': merged_at, + 'lastEditedAt': None, + 'additions': 10, + 'deletions': 5, + 'commits': {'totalCount': 1}, + 'baseRefName': 'main', + 'baseRefOid': 'abc', + 'headRefName': 'feature', + 'headRefOid': 'def', + 'bodyText': '', + 'closingIssuesReferences': {'nodes': []}, + 'changesRequestedReviews': {'nodes': []}, + 'labels': {'nodes': []}, + 'timelineItems': {'nodes': []}, + } + + +@patch('gittensor.utils.github_api_tools.bt.logging') +def test_stale_merged_pr_goes_to_storage_only_bucket(_): + miner_eval = MinerEvaluation(uid=74, hotkey='hk', github_id='1', github_pat='fake') + now = datetime.now(timezone.utc) + lookback = now - timedelta(days=35) + stale_merge = (lookback - timedelta(days=20)).strftime('%Y-%m-%dT%H:%M:%SZ') + created = (lookback - timedelta(days=40)).strftime('%Y-%m-%dT%H:%M:%SZ') + + _maybe_add_stale_merged_pr(miner_eval, _merged_pr_node(7629, created, stale_merge), lookback) + + assert len(miner_eval.merged_pull_requests) == 0, 'stale merged PR must not enter scoring bucket' + assert len(miner_eval.stale_merged_pull_requests) == 1, 'stale merged PR must enter storage-only bucket' + assert miner_eval.stale_merged_pull_requests[0].number == 7629 + assert miner_eval.stale_merged_pull_requests[0].pr_state == PRState.MERGED + assert miner_eval.stale_merged_pull_requests[0].merged_at is not None + + +@patch('gittensor.utils.github_api_tools.bt.logging') +def test_stale_merged_pr_not_counted_in_totals(_): + miner_eval = MinerEvaluation(uid=74, hotkey='hk', github_id='1', github_pat='fake') + now = datetime.now(timezone.utc) + lookback = now - timedelta(days=35) + stale_merge = (lookback - timedelta(days=20)).strftime('%Y-%m-%dT%H:%M:%SZ') + created = (lookback - timedelta(days=40)).strftime('%Y-%m-%dT%H:%M:%SZ') + + _maybe_add_stale_merged_pr(miner_eval, _merged_pr_node(7629, created, stale_merge), lookback) + + assert miner_eval.total_merged_prs == 0 + assert miner_eval.total_open_prs == 0 + assert miner_eval.total_closed_prs == 0 + assert miner_eval.total_prs == 0 + + +@patch('gittensor.utils.github_api_tools.bt.logging') +def test_fresh_merged_pr_is_not_captured_as_stale(_): + """Regression: PRs merged within the lookback window must NOT enter the + stale-merged bucket — they belong on the normal scoring path. + """ + miner_eval = MinerEvaluation(uid=74, hotkey='hk', github_id='1', github_pat='fake') + now = datetime.now(timezone.utc) + lookback = now - timedelta(days=35) + fresh_merge = (now - timedelta(days=5)).strftime('%Y-%m-%dT%H:%M:%SZ') + created = (now - timedelta(days=10)).strftime('%Y-%m-%dT%H:%M:%SZ') + + _maybe_add_stale_merged_pr(miner_eval, _merged_pr_node(1, created, fresh_merge), lookback) + + assert len(miner_eval.stale_merged_pull_requests) == 0 + + +@patch('gittensor.utils.github_api_tools.bt.logging') +def test_merged_pr_without_merged_at_is_skipped(_): + """Defensive: malformed MERGED PR with no mergedAt must not crash and must + not be captured (we cannot prove it's stale).""" + miner_eval = MinerEvaluation(uid=74, hotkey='hk', github_id='1', github_pat='fake') + now = datetime.now(timezone.utc) + lookback = now - timedelta(days=35) + created = (lookback - timedelta(days=40)).strftime('%Y-%m-%dT%H:%M:%SZ') + + pr = _merged_pr_node(2, created, merged_at='') + pr['mergedAt'] = None + + _maybe_add_stale_merged_pr(miner_eval, pr, lookback) + + assert len(miner_eval.stale_merged_pull_requests) == 0 + + +@patch('gittensor.utils.github_api_tools.bt.logging') +def test_multiple_stale_merged_prs_accumulate(_): + miner_eval = MinerEvaluation(uid=74, hotkey='hk', github_id='1', github_pat='fake') + now = datetime.now(timezone.utc) + lookback = now - timedelta(days=35) + stale_merge = (lookback - timedelta(days=20)).strftime('%Y-%m-%dT%H:%M:%SZ') + created = (lookback - timedelta(days=40)).strftime('%Y-%m-%dT%H:%M:%SZ') + + for i in range(3): + _maybe_add_stale_merged_pr(miner_eval, _merged_pr_node(200 + i, created, stale_merge), lookback) + + assert len(miner_eval.stale_merged_pull_requests) == 3 + assert miner_eval.total_merged_prs == 0 + assert miner_eval.total_prs == 0 diff --git a/tests/validator/utils/test_storage_mirror.py b/tests/validator/utils/test_storage_mirror.py index c516b7327..a8e2e6ce0 100644 --- a/tests/validator/utils/test_storage_mirror.py +++ b/tests/validator/utils/test_storage_mirror.py @@ -86,6 +86,7 @@ def _make_storage_with_mock_repo(): mock_repo = MagicMock() mock_repo.set_miner.return_value = 1 mock_repo.store_pull_requests_bulk.return_value = 0 # actual count irrelevant + mock_repo.refresh_stale_merged_pr_states.return_value = 0 mock_repo.refresh_stale_pr_states.return_value = 0 mock_repo.store_issues_bulk.return_value = 0 mock_repo.store_file_changes_bulk.return_value = 0 @@ -186,6 +187,52 @@ def test_stale_closed_prs_are_stored_separately(self): for call in mock_repo.store_pull_requests_bulk.call_args_list: assert not any(pr.number == 7 for pr in call.args[0]) + def test_stale_merged_prs_use_targeted_refresh_path(self): + """Stale-MERGED PRs route to ``refresh_stale_merged_pr_states`` (a + targeted UPDATE) rather than the full ``store_pull_requests_bulk`` + upsert, so historical scoring fields on already-MERGED rows are not + clobbered with dataclass defaults. + """ + storage, mock_repo = _make_storage_with_mock_repo() + + eval_ = MinerEvaluation(uid=1, hotkey='hk', github_id='123') + eval_.stale_merged_pull_requests = [_legacy_pr(7629, state=PRState.MERGED)] + + storage.store_evaluation(eval_) + + # Targeted refresh, not a bulk upsert. + mock_repo.refresh_stale_merged_pr_states.assert_called_once() + refresh_arg = mock_repo.refresh_stale_merged_pr_states.call_args.args[0] + assert len(refresh_arg) == 1 + assert refresh_arg[0].number == 7629 + assert refresh_arg[0].pr_state == PRState.MERGED + assert refresh_arg[0].merged_at is not None + + # Stale-merged storage must not inflate scoring totals. + assert eval_.total_merged_prs == 0 + assert eval_.total_prs == 0 + + # And it must not be passed to the bulk upsert path + # (3 calls: merged, open, closed). Stale-closed PRs go through + # refresh_stale_pr_states, not the bulk upsert. + assert mock_repo.store_pull_requests_bulk.call_count == 3 + for call in mock_repo.store_pull_requests_bulk.call_args_list: + assert call.args[0] == [] or call.args[0][0].number != 7629 + + def test_empty_stale_merged_list_still_invokes_refresh(self): + """Even with no stale-merged PRs, the refresh call happens (returns 0) + so the storage transaction shape stays predictable across rounds. + """ + storage, mock_repo = _make_storage_with_mock_repo() + + eval_ = MinerEvaluation(uid=1, hotkey='hk', github_id='123') + # stale_merged_pull_requests left empty by default + + storage.store_evaluation(eval_) + + mock_repo.refresh_stale_merged_pr_states.assert_called_once() + assert mock_repo.refresh_stale_merged_pr_states.call_args.args[0] == [] + def test_cleanup_stale_called_with_commit_false(): """cleanup_stale_miner_data must be called with commit=False inside the transaction.