Skip to content

fix(validator): count only canonical valid solved issues#1342

Closed
Khaostica wants to merge 2 commits into
entrius:testfrom
Khaostica:fix/issue-1269-canonical-valid-solved-v2
Closed

fix(validator): count only canonical valid solved issues#1342
Khaostica wants to merge 2 commits into
entrius:testfrom
Khaostica:fix/issue-1269-canonical-valid-solved-v2

Conversation

@Khaostica
Copy link
Copy Markdown

@Khaostica Khaostica commented May 22, 2026

Summary

Move acc.valid_solved += 1 in _score_miner_issues past every credibility-only gate (same-account, one-issue-per-PR canonical, per-repo token threshold). Previously the increment fired on token-score alone, before the canonical check, so seven sibling issues closed by one qualifying PR could satisfy the per-repo MIN_VALID_SOLVED_ISSUES eligibility gate even though only one was scoreable.

Adds regression coverage for both the single-repo and per-repo (#1293) shapes, plus a credibility-only assertion on the same-account path, and updates two existing tests whose assertions/fixtures encoded the buggy "seven-counts-as-seven" behavior.

This is a rebased, refactor-aware re-do of #1273 (closed for unresolved conflicts after #1293 landed feat(validator): per-repository eligibility). The original patch was written against the pre-refactor global valid_solved_count / MIN_TOKEN_SCORE_FOR_BASE_SCORE; this one targets acc.valid_solved / cfg.min_token_score_for_valid_issue and also moves the increment after the token-threshold quality gate so below-threshold canonical issues no longer inflate the count.

Related Issues

Fixes #1269. Supersedes #1273.

Type of Change

  • Bug fix
  • New feature
  • Refactor
  • Documentation
  • Other (describe below)

Testing

  • Tests added/updated
  • Manually tested

Local verification:

Test changes:

  • test_self_issue_counts_credibility_but_no_score — adds total_valid_solved_issues == 0 assertion (same-account is credibility-only, must not bump the gate).
  • test_two_miners_shared_pr_only_earliest_scores — B now has total_valid_solved_issues == 6 (was 7, the bug). B stays eligible since MIN_VALID_SOLVED_ISSUES was dropped to 3 since [Bug] Non-canonical one-issue-per-PR siblings still satisfy the valid-solved eligibility gate #1269 was filed; the dedicated eligibility-gate regression lives in the new sibling test below.
  • test_within_miner_one_issue_per_pr_still_holds — miner now has total_valid_solved_issues == 7 (was 8). Still eligible (7 unique canonical valid solved).
  • test_all_mirror_miner_below_threshold_passes_spam — existing test that relied on the bug (eight issues sharing one PR all counted as valid_solved). Gave each issue its own solving PR and seeded all eight so the spam-mult assertion still has an eligible miner post-fix.
  • New test_non_canonical_siblings_do_not_satisfy_valid_solved_gate — direct [Bug] Non-canonical one-issue-per-PR siblings still satisfy the valid-solved eligibility gate #1269 repro: seven sibling issues, one canonical PR → valid_solved == 1, ineligible (1 < 3), score == 0.
  • New test_non_canonical_siblings_per_repo_eligibility — multi-repo variant: 7 unique canonical issues in repo A and 7 siblings on one shared PR in repo B → only repo A contributes to scored issues; total valid_solved == 8 (7 + 1 canonical sibling).

Checklist

  • Code follows project style guidelines
  • Self-review completed
  • Changes are documented (if applicable)

Move acc.valid_solved += 1 past every credibility-only gate (same-account,
one-issue-per-PR canonical, token threshold). Previously the increment fired
on token-score alone, before the canonical check, so seven sibling issues
closed by one qualifying PR could satisfy the per-repo MIN_VALID_SOLVED_ISSUES
eligibility gate even though only one was scoreable.

Adds regression coverage for both single-repo and per-repo eligibility,
plus a credibility-only assertion for the same-account path, and updates
two existing tests whose assertions encoded the buggy 7-counts-as-7 behavior.

Fixes entrius#1269.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@xiao-xiao-mao xiao-xiao-mao Bot added the bug Something isn't working label May 22, 2026
Two test fixes uncovered by actually running pytest locally:

1. test_two_miners_shared_pr_only_earliest_scores — MIN_VALID_SOLVED_ISSUES
   dropped from 7 to 3 since entrius#1269 was filed, so B (valid_solved == 6 after
   the canonical fix) remains eligible. Restore the 7:6 score-ratio
   assertion; the dedicated eligibility-gate regression lives in
   test_non_canonical_siblings_do_not_satisfy_valid_solved_gate where
   valid_solved == 1 < 3.

2. test_all_mirror_miner_below_threshold_passes_spam — this test relied on
   eight sibling issues sharing one solving PR all counting as
   valid_solved (the entrius#1269 bug). Give each issue its own solving PR and
   seed all eight so the spam-mult assertion still has an eligible miner
   to assert against post-fix.

All 56 tests in tests/validator/issue_discovery/ now pass locally.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Khaostica Khaostica marked this pull request as ready for review May 22, 2026 15:16
@anderdc
Copy link
Copy Markdown
Collaborator

anderdc commented May 23, 2026

valid_solved is the can't-parse fallback for issues whose discovery was credited by a qualifying PR — it's intentionally broader than the score-row set, which is what the canonical one-issue-per-PR check governs. In practice a PR closes at most a handful of issues; when it closes more, the maintainer intake on the issues is the primary control, not the validator-side counter. Stripping siblings here drops eligibility credit on routine cross-miner collisions and batch-fix PRs (Fixes #X #Y #Z) where honest discoverers share a solving PR. Closing.

@anderdc anderdc closed this May 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Non-canonical one-issue-per-PR siblings still satisfy the valid-solved eligibility gate

3 participants