Skip to content

[Bug] Three gaps in #769 stale-closed PR fix: scoring data loss, silent drop path, and fragile test #991

Description

@dataCenter430

Description

PR #769 introduced the stale_closed_pull_requests bucket to refresh pr_state for out-of-window PRs but left two issues unaddressed.

Gap 1 — Stale UPSERT silently zeroes previously-scored rows
store_pull_requests_bulk in gittensor/validator/utils/storage.py:74-76 uses BULK_UPSERT_PULL_REQUESTS, an ON CONFLICT DO UPDATE SET that overwrites all columns via EXCLUDED.*. PullRequest.from_graphql_response() leaves every scoring field at its dataclass default (base_score = 0.0, earned_score = 0.0, credibility_multiplier = 1.0, etc.), so any row that was previously scored has those computed values silently replaced with zeros on every subsequent scan.

Gap 2 — Fragile positional assertion in test_stale_closed_prs_are_stored_separately
tests/validator/utils/test_storage_mirror.py:176 asserts stale storage via a hardcoded call index call_args_list[3]. Any reordering of bulk calls in store_evaluation silently breaks the assertion's target without failing the test.

Steps to Reproduce

  1. A miner has a PR that was previously scored (e.g. was OPEN with collateral) and later closed outside the 35-day lookback window.
  2. On the next validator scan, try_add_open_or_closed_pr routes it to stale_closed_pull_requests.
  3. store_pull_requests_bulk fires the full BULK_UPSERT_PULL_REQUESTS with dataclass-default scoring fields.
  4. The previously-computed earned_score, base_score, credibility_multiplier, etc. are overwritten with 0.0 / 1.0 in the database.

Expected Behavior

  • Only pr_state and updated_at should be modified for stale PRs. All scoring columns on existing rows must be left untouched.
  • The stale storage test should assert against the dedicated stale method by name, not by positional call index.

Actual Behavior

  • Previously-scored rows have their scoring columns zeroed out by the stale UPSERT on every subsequent scan.
  • test_stale_closed_prs_are_stored_separately asserts via call_args_list[3] and would pass incorrectly if call order in store_evaluation changed.

Environment

Additional Context

Follow-up to #769, #768. No schema changes are required to fix either gap. Gap 1 can be resolved with a dedicated method backed by a targeted UPDATE touching only pr_state and updated_at instead of routing through the full-column UPSERT path.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions