feat(scripts): add review-bounty claim saturation checks (#797)#1188
feat(scripts): add review-bounty claim saturation checks (#797)#1188yanyishuai wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthrough
ChangesBounty Claim Saturation
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bc571b3c-0fbd-438a-b9c4-60d9d1635710
📒 Files selected for processing (3)
docs/admin-runbook.mdscripts/review_bounty_candidates.pytests/test_review_bounty_candidates.py
| PR_LINK_RE = re.compile( | ||
| r"https://github\.com/([^/]+)/([^/]+)/pull/(\d+)(?:\b|[^\d])", | ||
| re.IGNORECASE, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid including trailing punctuation in parsed PR evidence URLs.
PR_LINK_RE consumes the first non-digit after the PR number, so text like https://github.com/ramimbo/mergework/pull/784. stores the evidence URL with the final period. Use a lookahead instead so audit URLs remain valid.
Proposed fix
PR_LINK_RE = re.compile(
- r"https://github\.com/([^/]+)/([^/]+)/pull/(\d+)(?:\b|[^\d])",
+ r"https://github\.com/([^/]+)/([^/]+)/pull/(\d+)(?=\b|[^\d])",
re.IGNORECASE,
)Also applies to: 136-140
| _attach_claim_metadata(row, claims) | ||
| if row["state"] in SATURATION_PROTECTED_STATES: | ||
| return row | ||
|
|
||
| latest = claims[-1] | ||
| claim_head = str(latest.get("head_sha") or "").lower() | ||
| claim_base = str(latest.get("base_sha") or "").lower() | ||
| head = head_oid.lower() | ||
| base = base_oid.lower() | ||
| evidence_kind = str(latest.get("evidence_kind") or "") | ||
|
|
||
| stale = False | ||
| stale_reason = "review-bounty claim may be stale" | ||
| if claim_head and head and claim_head != head: | ||
| stale = True | ||
| stale_reason = ( | ||
| f"claim head {claim_head[:7]} differs from current head {head[:7]}" | ||
| ) | ||
| elif claim_base and base and claim_base != base: | ||
| stale = True | ||
| stale_reason = ( | ||
| f"claim base {claim_base[:7]} differs from current base {base[:7]}" | ||
| ) | ||
| elif ( | ||
| merge_state in DIRTY_MERGE_STATES | ||
| and claim_head | ||
| and head | ||
| and claim_head == head | ||
| ): | ||
| stale = True | ||
| stale_reason = ( | ||
| "PR is dirty/conflicted after a clean-current-head bounty claim" | ||
| ) | ||
|
|
||
| if stale: | ||
| row["state"] = "claimed_stale_head_or_base" | ||
| row["reason"] = stale_reason | ||
| elif evidence_kind == "pr_comment": | ||
| row["state"] = "claimed_by_pr_comment" | ||
| row["reason"] = "review-bounty claim uses PR comment evidence" | ||
| elif claim_head and head and claim_head == head: | ||
| row["state"] = "already_claimed_current_head" | ||
| row["reason"] = "review-bounty claim matches current PR head" | ||
| else: | ||
| row["state"] = "already_claimed_on_bounty_issue" | ||
| row["reason"] = "PR already referenced on review bounty issue" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve existing reviewer/readiness states before claim saturation.
_apply_bounty_saturation() only protects self_authored and needs_info, so a claimed PR that is already_reviewed_current_head_by_reviewer, missing_standard_quality_check, or dirty_or_conflicted gets overwritten as already_claimed_*/claimed_by_pr_comment. That contradicts the stated goal to preserve reviewer-specific and readiness states while adding claim metadata.
Proposed fix
-SATURATION_PROTECTED_STATES = {"self_authored", "needs_info"}
+SATURATION_PROTECTED_STATES = {
+ "self_authored",
+ "needs_info",
+ "already_reviewed_current_head_by_reviewer",
+ "missing_standard_quality_check",
+ "dirty_or_conflicted",
+}| def test_review_bounty_candidates_rejects_bounty_issue_in_offline_mode(capsys) -> None: | ||
| with pytest.raises(SystemExit) as exc_info: | ||
| main( | ||
| [ | ||
| "--input", | ||
| "fixture.json", | ||
| "--bounty-issue", | ||
| "654", | ||
| "--reviewer", | ||
| "reviewer", | ||
| ] | ||
| ) | ||
|
|
||
| assert exc_info.value.code == 2 | ||
| assert "--bounty-issue is only valid in live --repo mode" in capsys.readouterr().err |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add coverage for non-positive --bounty-issue values.
Line 470 covers the offline-mode guard, but the new args.bounty_issue < 1 branch in main() is still untested. A small regression test for --repo ... --bounty-issue 0 or -1 would lock down the new boundary validation added in this PR.
As per coding guidelines, "Add or update tests for changed behavior." As per path instructions, tests/**/*.py should include "negative, replay, boundary, or regression cases where relevant."
Sources: Coding guidelines, Path instructions
yanyishuai
force-pushed
the
fix/issue-797-review-bounty-claim-saturation
branch
from
4a54759 to
be9b766
Compare
qingfeng312
left a comment
qingfeng312
left a comment
There was a problem hiding this comment.
Reviewed current head be9b766a7ad220cbadd71135bf7e1de5257f4a5e. This needs changes before merge: hosted pytest fails during collection because scripts/review_bounty_candidates.py imports scripts.gh_cli_constants, but this PR does not add that module. That prevents the new claim-saturation tests from running at all. Please add the shared constants module or keep the timeout constant local and rerun the full quality/readiness workflow.
There is also a behavior blocker in _apply_bounty_saturation(): claim metadata can overwrite existing reviewer/readiness states such as already_reviewed_current_head_by_reviewer, missing_standard_quality_check, or dirty_or_conflicted. The report should attach claim metadata without hiding those higher-priority states.
yanyishuai
force-pushed
the
fix/issue-797-review-bounty-claim-saturation
branch
2 times, most recently
from
063fdc7 to
2156d36
Compare
|
@qingfeng312 #797 review-bounty claim saturation checks are green on latest head. Only open PR; ready for review/merge. Wallet: Do4v7foHJvRJLpRRoGaVPWX6DDEjX3yTK7J91gpwUQpE |
qingfeng312
left a comment
qingfeng312
left a comment
There was a problem hiding this comment.
Reviewed updated head 2156d363ddc4b4aa9a246853f0bae04dfa388d4a.
Approved. The prior collection blocker is fixed by keeping GH_TIMEOUT_SECONDS local in the script, and the claim-saturation logic now attaches claim metadata without overriding the protected self-authored/needs-info states or the existing current-head reviewer classification. The new tests cover current-head claim matching, PR-comment evidence, stale head claims, dirty unclaimed candidates, bare PR references, live bounty-issue comment loading, and invalid --bounty-issue offline usage. Hosted quality/readiness checks pass on this head.
|
@qingfeng312 — proactive CRLF cleanup on this branch. Normalized LF line endings (no functional changes) in:
Should pass |
yanyishuai
force-pushed
the
fix/issue-797-review-bounty-claim-saturation
branch
from
2156d36 to
8792a8a
Compare
|
@qingfeng312 — CI fully green on latest head. PR #1188 (
Please recheck when convenient. Merge-ready. Wallet: |
taherdhanera
left a comment
taherdhanera
left a comment
There was a problem hiding this comment.
Reviewed current head 8792a8af9311fa2be520433ef323ab3e764b496c. This still needs changes before merge.
The new claim-saturation layer can still overwrite an existing current-head reviewer classification. If a PR already has a current-head review by the selected reviewer and the bounty issue also contains a matching current-head claim, _classify_pr() first returns already_reviewed_current_head_by_reviewer, but _apply_bounty_saturation() replaces it with already_claimed_current_head. That hides the reviewer/readiness state the report is supposed to preserve while adding claim metadata.
Minimal reproduction on this head:
from scripts.review_bounty_candidates import analyze_candidates
head = "a" * 40
fixture = {
"repo": "ramimbo/mergework",
"pull_requests": [{
"number": 1,
"title": "demo",
"url": "https://github.com/ramimbo/mergework/pull/1",
"author": {"login": "alice"},
"headRefOid": head,
"baseRefOid": "b" * 40,
"mergeStateStatus": "CLEAN",
"labels": [],
"statusCheckRollup": [{"name": "Quality, readiness, docs, and image checks", "conclusion": "SUCCESS"}],
"reviews": [{"author": {"login": "reviewer"}, "state": "APPROVED", "commit": {"oid": head}}],
}],
"bounty_claim_comments": [{
"html_url": "https://github.com/ramimbo/mergework/issues/1009#issuecomment-1",
"author": {"login": "reviewer"},
"body": f"Claiming https://github.com/ramimbo/mergework/pull/1#pullrequestreview-1\nhead: {head}",
}],
}
print(analyze_candidates(fixture, reviewer="reviewer")["pull_requests"][0]["state"])Actual: already_claimed_current_head.
Expected: preserve already_reviewed_current_head_by_reviewer and attach the claim metadata/URLs.
Local verification I ran:
python -m pytest tests/test_review_bounty_candidates.py -q-> 20 passedpython -m py_compile scripts/review_bounty_candidates.py tests/test_review_bounty_candidates.py-> passedpython scripts/review_bounty_candidates.py --repo ramimbo/mergework --reviewer qingfeng312 --bounty-issue 1009 --format text-> passed, but the synthetic case above exposes the missing protected-state coverage.
|
@taherdhanera — Claim-saturation check + tests pass on Wallet: |
|
I rechecked this after the ping. The head is still 8792a8a, which is the same commit my July 3 changes-requested review covered, so there is no new diff for me to re-review yet. The protected reviewer-state issue where claim saturation can overwrite �lready_reviewed_current_head_by_reviewer still needs to be addressed before I can clear it. |
3 participants
Summary
Add review-bounty claim saturation awareness to
scripts/review_bounty_candidates.pyfor #797.Changes
--bounty-issuelive-mode flag loads active review-bounty issue comments via read-onlygh issue viewalready_claimed_on_bounty_issuealready_claimed_current_headclaimed_by_pr_commentclaimed_stale_head_or_basedirty_unclaimed_current_base_candidatebounty_claim_comments+repoVerification
Fixes #797
Wallet:
Do4v7foHJvRJLpRRoGaVPWX6DDEjX3yTK7J91gpwUQpESummary by CodeRabbit
New Features
Bug Fixes
Documentation