fix: apply review_quality_multiplier to open PR collateral

[Crystora]

Summary

calculate_open_pr_collateral_score applied repo_weight, issue, and label multipliers but silently ignored review_quality_multiplier. An open PR with maintainer CHANGES_REQUESTED reviews was charged the same collateral as a clean PR, overstating the merged earned score it is meant to approximate.

Three coupled sites were hiding the bug:

1. gittensor/classes.py - changes_requested_count was gated behind if is_merged:, so open PRs got 0 even though the GraphQL query already fetches changesRequestedReviews for every PR.
2. gittensor/validator/oss_contributions/scoring.py - calculate_pr_multipliers hardcoded pr.review_quality_multiplier = 1.0 in the open-PR branch.
3. Same file - calculate_open_pr_collateral_score omitted review_quality from its multipliers dict.

Changes

• Extract _count_maintainer_changes_requested in classes.py and call it unconditionally so both merged and open PRs get a real count.
• Lift review_quality_multiplier out of the if is_merged: branch in calculate_pr_multipliers.
• Add 'review_quality' to the multipliers dict in calculate_open_pr_collateral_score and update its docstring.

No GraphQL changes - changesRequestedReviews is already fetched inline (#519), so there is no new API cost.

Related precedents

• fix: include label_multiplier in open PR collateral score #613 - added label_multiplier to open-PR collateral (same shape)
• fix: apply is_valid_issue state_reason gate to open PR collateral #691 - extended is_valid_issue state_reason gate to open-PR collateral (same shape)

Related Issues

Closes #744

Type of Change

• Bug fix

Testing

• Tests added/updated
• Manually tested

21 tests in tests/validator/test_review_quality_multiplier.py pass. New coverage:

• TestChangesRequestedCountFromGraphQL.test_open_pr_also_counts_maintainer_reviews - flips the old assertion; open PRs now parse changesRequestedReviews.
• TestReviewQualityMultiplierOnOpenPRCollateral - direct collateral math under clean, penalized, and zero-multiplier conditions.
• TestReviewQualityMultiplierThroughScoringPipeline - end-to-end regression guards:
  • calculate_pr_multipliers applies review_quality to open PRs
  • merged and open PRs derive the same multiplier from the same CR count
  • open-PR collateral equals the projected merged earned score × OPEN_PR_COLLATERAL_PERCENT

Full validator suite: 254 passed. ruff check, ruff format, and pyright all clean.

[Crystora]

@anderdc Pls review this PR.

[anderdc]

We generally just add 'multipliers' to the collateral. I'm not sure if review penalty should be added. Let me chew on it

[Crystora]

We generally just add 'multipliers' to the collateral. I'm not sure if review penalty should be added. Let me chew on it

review_quality is also part of the merged-PR formula. It just wasn't being applied to collateral. So the open-PR projection didn't match the merged-PR reality - that's the inconsistency I'm fixing.

Adding review_quality to collateral actually lowers the deduction on PRs with changes-requested reviews. It's not a new penalty. Clean PRs (no CR reviews) see zero change.

The reason time_decay and credibility aren't in collateral is they need merge data we don't have yet. review_quality doesn't have that problem - the data is already there for open PRs.

[anderdc]

The point of collateral is to 'put your money where your mouth is' on a PR, with this addition you're more ok with review iterations because it eases up on your collateral (at the cost of your resulting PR). So still debating, the argument isn't particularly compelling but also not dismissed. Any more thoughts?

[Crystora]

Fair point. But the thing is, collateral already multiplies by repo_weight, issue, and label today. So a refactor PR (0.5×) already pays less collateral than a feature PR (1.5×) for the same work. By the "fixed deposit" logic, those probably shouldn't be there either.
I just wanted review_quality to match what the other three already do. If you'd rather pull all four out and make collateral a flat base × 20%, I'm happy to flip the PR that way instead. Just let me know which option you want.

[Crystora]

On the "more ok with review iterations" concern - I don't think it actually plays out that way.

The case we care about deterring is someone opening a bunch of speculative PRs and walking away. Those PRs haven't been touched by a maintainer yet, so review_quality is 1.0 and their collateral is unchanged. The spam protection still works the same way.

What does change is collateral on PRs that a maintainer actively reviewed and asked for changes on. Today those get hit twice: full collateral while they're open, AND lower earned score if they merge. With this change they get hit once, and the open-side collateral matches what the merged PR will actually be worth.

On the multipliers question more broadly: collateral today isn't a flat deposit, it's base × repo × issue × label × 20%. So it's already trying to estimate merged value. review_quality is just another part of that estimate. Either all four belong or none do - having three of them is what feels arbitrary.

#613 (label_multiplier) and #691 (state_reason gate) added missing pieces to collateral for the same consistency reason, so there's precedent.

If you'd actually prefer to strip all four and make collateral flat base × 20%, I can open that as a separate PR. But this one is the smaller step in the direction collateral already goes.

[LandynDev]

We should not make collateral less for an open PR that has requested changes. If we do this, we are incentivizing open PRs to go through iterations as it becomes less of a tax on miners. I'd prefer if the inverse happened and it became more of a tax, if anything. The goal is to find ways to have the network produce consistently merge-able PRs from the second they're made (0 iterations/requested changes), this doesn't align with that goal.

Yes, it's more 'accurate' and 1:1 with what the merged score would be.
But miner failing to do their job (perfect pr with 0 iterations) should not alleviate the collateral burden imo

[Crystora]

The goal is to find ways to have the network produce consistently merge-able PRs from the second they're made (0 iterations/requested changes), this doesn't align with that goal.

@LandynDev Thanks for clarifying. I agree collateral should not become lower when a PR gets requested changes. If not, it can be another spam gap.
Instead of reducing the collateral, I’ll consider increasing when requesting changes:
Simple direction I propose:

• clean PR: no change
• PR with requested changes: higher collateral, not lower.

review_penalty_count = number of maintainer CHANGES_REQUESTED reviews
review_collateral_multiplier = 1 + (review_penalty_count × 0.15)
open_pr_collateral =
base_score × repo_weight × issue × label × review_collateral_multiplier × 20%

Example:
0 requested changes:
100 × 1.00 × 20% = 20

1 requested change:
100 × 1.15 × 20% = 23

2 requested changes:
100 × 1.30 × 20% = 26

3 requested changes:
100 × 1.45 × 20% = 29

I would also cap it so it cannot grow forever:
review_collateral_multiplier = min(2.0, 1 + requested_changes_count × 0.15)
So max collateral from review iterations is 2x the normal collateral.

That keeps the incentive aligned with merge-ready PRs while still using the review quality signal.

---

What do you think about it? @anderdc @LandynDev
I'd appreciate any feedback

[anderdc]

Let's actually do that and see how it goes, that makes more sense to me. The goal is mergeable quality PRs from the get-go.

[Crystora]

@anderdc Ready to review

[anderdc]

no need for another separate constant for this, let's just make them both use REVIEW_PENALTY_RATE

[Crystora]

@anderdc I've updated. Pls check again.