fix: use total solved_count for issue credibility instead of valid_solved_count#643
Conversation
anderdc
left a comment
There was a problem hiding this comment.
Fix is correct — the bug is real, the math in the issue is verified, and the approach matches the existing OSS credibility pattern cited in #642. Just requesting a test before merge.
Why a test matters for this one:
This is a scoring-formula change with direct impact on every miner's issue credibility. The bug was introduced by a silent parameter collapse during a previous refactor (solved_count and valid_solved_count got merged into one slot), and without a test locking down the corrected behavior, the same class of regression could happen again on the next refactor.
Suggested test shape (~15 lines in tests/validator/test_issue_discovery_scoring.py or similar):
@pytest.mark.parametrize(
'solved_count,valid_solved_count,closed_count,expected_credibility,expected_eligible',
[
# Issue #642 example: 10 total solved, 7 valid, 2 closed
# Credibility uses total (10), eligibility gate uses valid (7)
(10, 7, 2, pytest.approx(10 / 11, abs=1e-3), True),
# Valid count below threshold → not eligible regardless of credibility
(10, 6, 2, pytest.approx(10 / 11, abs=1e-3), False),
# Zero solved → credibility 0, not eligible
(0, 0, 2, 0.0, False),
# All solved counts equal (no low-token solvers) → fix is no-op for this case
(7, 7, 2, pytest.approx(7 / 8, abs=1e-3), True),
],
)
def test_check_issue_eligibility_uses_total_for_credibility_valid_for_gate(
solved_count, valid_solved_count, closed_count, expected_credibility, expected_eligible
):
is_eligible, credibility, _ = check_issue_eligibility(solved_count, valid_solved_count, closed_count)
assert credibility == expected_credibility
assert is_eligible is expected_eligibleThat locks down:
- The 10/7/2 example from the issue body
- The valid-below-threshold case (eligibility fails even with good credibility)
- The zero-input edge case
- The
solved_count == valid_solved_countcase (no-op for miners without low-token solvers)
Parametrizes cleanly, covers the branches, and documents the intent for future refactorers.
Everything else looks good — scope is minimal (4 code lines), approach matches the OSS pattern, signature change is local, and the caller is updated correctly. Once the test lands, ready to merge.
…lved_count `check_issue_eligibility` received `data.valid_solved_count` as its first parameter and used it for BOTH the eligibility minimum check and the credibility formula. Per the docs, credibility should use ALL solved issues (not just those whose solving PR has token_score >= 5). The OSS credibility implementation (`oss_contributions/credibility.py:49-52`) separates these correctly. Example from entrius#642: a miner with 10 solved (7 valid, 3 low-token) and 2 closed issues gets credibility 7/8 = 0.875 today, but the correct value is 10/11 = 0.91 — a 4% scoring penalty for having solved issues with small PRs. Splits `check_issue_eligibility` to accept `(solved_count, valid_solved_count, closed_count)`: credibility uses `solved_count`, the minimum-issues gate uses `valid_solved_count`. Caller in `score_discovered_issues` is updated to pass both. Adds a parametrized regression test locking the corrected behavior per the maintainer's review of entrius#643: 4 cases covering the entrius#642 example, valid-below-threshold, zero input, and the no-op case where `solved_count == valid_solved_count`. Fixes entrius#642.
5b27127 to
a4c615b
Compare
…ate split Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
a4c615b to
6aee1ce
Compare
631f182 to
c312247
Compare
|
Hi, @anderdc |
|
ci failed |
30b9bc9 to
ca60069
Compare
|
Hi, @anderdc |
|
🐛 CI Bug was fixed Hi, @anderdc |
Origin/test's #643 changed check_issue_eligibility to take (solved_count, valid_solved_count, closed_count) — previously (valid_solved_count, closed_count). Mirror issue discovery call site needed the same update to pass solved_count through.
Summary
solved_countandvalid_solved_counttocheck_issue_eligibilitysolved_countfor credibility formula (matches docs and OSS pattern)valid_solved_countfor the minimum count gate (unchanged behavior)evaluation.issue_credibilityto reflect the corrected valueCloses #642
Problem
check_issue_eligibilityreceivesvalid_solved_countand uses it for both the eligibilitygate and the credibility formula. The credibility formula should use ALL solved issues, not
just valid ones. This matches the OSS credibility pattern which passes all merged PRs to
calculate_credibilityand separately counts valid ones.Test plan