fix: improve review quality multiplier observability + harden is_valid_issue - #743
Conversation
anderdc
left a comment
There was a problem hiding this comment.
remove the database changes
|
Thanks for your review, @anderdc |
|
Just done and ready to review! |
03f18b4 to
c00ac34
Compare
|
Hi, @anderdc |
anderdc
left a comment
There was a problem hiding this comment.
Prior DB-changes complaint is resolved. Two new asks before merge:
-
The body claims
abs()replaced the< 0sign check ondays_diff, but the actual diff atscoring.py:482keepsif days_diff > MAX_ISSUE_CLOSE_WINDOW_DAYS or days_diff < 0:unchanged. Fix the description or actually make the change. -
Please scope down to #742. Keep: the
pr_numberparameter oncalculate_review_quality_multiplierand its call site incalculate_pr_multipliers. Drop: theis_valid_issueclose-window changes — that's unrelated to #742 and is its own scoring-eligibility decision, file separately. If you want to also pick up #742 ask 3 (logging incalculate_issue_review_quality_multiplier), do it here; otherwise note it as a follow-up.
There was a problem hiding this comment.
Prior DB-changes ask is resolved. One ask before merge: please scope this down to #742.
Keep: the pr_number parameter on calculate_review_quality_multiplier and its call site in calculate_pr_multipliers.
Drop: the is_valid_issue close-window changes — unrelated to #742 and a separate scoring-eligibility decision.
If you also want to pick up #742 ask 3 (logging in calculate_issue_review_quality_multiplier), include it here; otherwise note it as a follow-up.
|
Okay, thanks for your review. |
837fb05 to
23e43e8
Compare
Summary
Two fixes merged in this PR:
1. Review quality multiplier observability
calculate_review_quality_multiplierlogged penalties without the PR number, making log lines untraceable during a validator run. Added optionalpr_numberparam (same pattern as PR fix: add PR number context to file-content fetch failure warning #711) so the call site incalculate_pr_multiplierscan passpr.number.2. Harden
is_valid_issueclose-window checkThe original guard was
if issue.closed_at and pr.merged_at:— silently skipping issues with a missingclosed_atinstead of flagging them. Two changes:closed_atguard — issues that areCLOSEDbut have noclosed_attimestamp now log a warning and are rejected instead of being silently accepted.abs()on days_diff — the sign check (days_diff < 0) was replaced withabs()so the window is symmetric and the rejection log no longer shows a signed delta.Test plan
(PR #N)in review quality penalty linesclosed_attimestamp are rejected with a warningis_valid_issuetests passCloses: #742