Add review bounty claim saturation checks#841
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds extraction of structured bounty-claim evidence from issue comments, indexes claims by PR, passes claims into PR classification (new claim-driven states and metadata), appends claim links in text/markdown reports, supports live paginated GH comment loading via CLI, and adds unit/integration tests. ChangesBounty Claim Evidence and Classification
Proposed Work Triage
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: ce2d8b74-1045-4074-bc03-d497de3bbead
📒 Files selected for processing (2)
scripts/review_bounty_candidates.pytests/test_review_bounty_candidates.py
Zejia
left a comment
Zejia
left a comment
There was a problem hiding this comment.
I think this needs one correctness fix before merge.
The new claim saturation logic accepts 7-40 character SHAs in HEAD_SHA_RE, but _classify_pr() only treats a claim as current-head when claim["head_sha"] == headRefOid exactly. That means common abbreviated or uppercase current-head SHAs are parsed successfully, then fail the current-head match and get weaker/stale classification.
Direct reproduction on current head f8cad3798db475f94c1a9e5cc0acf01ed965f11e:
- Claim body with
Head SHA: f8cad3798db4-> statealready_claimed_on_bounty_issue, notalready_claimed_current_head. - Claim body with uppercase full SHA
F8CAD3798DB475F94C1A9E5CC0ACF01ED965F11E-> statealready_claimed_on_bounty_issue, notalready_claimed_current_head. - Claim body with lowercase full SHA
f8cad3798db475f94c1a9e5cc0acf01ed965f11e-> statealready_claimed_current_head.
That is a real saturation problem because the parser advertises support for short SHA evidence, and short SHAs are common in review/claim comments. I would normalize case and compare using prefix semantics when the parsed SHA is shorter than the current full head, then add regression coverage for abbreviated and uppercase current-head claims.
Validation I ran:
python -m pytest tests/test_review_bounty_candidates.py -q-> 8 passedpython -m ruff check scripts/review_bounty_candidates.py tests/test_review_bounty_candidates.py-> passedpython -m ruff format --check scripts/review_bounty_candidates.py tests/test_review_bounty_candidates.py-> 2 files already formattedpython -m mypy scripts/review_bounty_candidates.py-> successgit diff --check origin/main...HEAD-> cleangit merge-tree --write-tree origin/main HEAD-> clean tree8a714b117b099657796936d27842d486555f65d9- Hosted Quality/readiness/docs/image and CodeRabbit checks are passing
No private data, secrets, wallet material, issue mutation, label mutation, payment action, treasury mutation, exchange, bridge, cash-out, or price behavior was used.
|
Thanks, that was a real correctness issue. Fixed in What changed:
Validation on the updated branch:
Hosted Quality and CodeRabbit are rerunning on the new head now. |
|
Follow-up on the reviewer-fix head
This should be ready for re-review on the SHA-normalization blocker. |
prettyboyvic
left a comment
prettyboyvic
left a comment
There was a problem hiding this comment.
Thanks for fixing the abbreviated/uppercase SHA comparison from the previous head. I found one remaining current-head blocker in the live claim-thread path.
When a PR has a stale claim on the active bounty issue and then receives a new head, the --bounty-issue report still suppresses it as already claimed instead of surfacing that the claim is stale and the new head needs review. That is the core case this helper is meant to make visible for review-bounty work.
Reproduction on current head c416855f8356b0c1f96f2c55fd3e9503c3eb8899 against current origin/main c504500cbe0b8c0acae692b27521ed778339d16e:
python scripts/review_bounty_candidates.py \
--repo ramimbo/mergework \
--bounty-issue 654 \
--reviewer prettyboyvic \
--format jsonThe row for this PR is:
headRefOid:c416855f8356b0c1f96f2c55fd3e9503c3eb8899- prior claim
head_sha:f8cad3798db475f94c1a9e5cc0acf01ed965f11e current_head_human_reviews:0latest_human_review.commit:f8cad3798db475f94c1a9e5cc0acf01ed965f11emergeStateStatus:clean- actual state emitted:
already_claimed_on_bounty_issue - reason emitted:
active bounty issue already references this PR
That means the helper would tell reviewers not to look at the current fixed head of #841 even though the only human review/claim evidence is for the old head. I think a clean PR with bounty claims whose parsed head_sha does not match the current headRefOid should remain eligible for follow-up review or be classified explicitly as stale-head/base evidence, rather than being treated as saturated just because the bounty issue mentions the PR.
The same live row also shows bounty_claim_count=2 for one Zejia claim comment: one pr_review claim from the PR review URL and one pr_reference claim from the same issue comment's "PR #841" text. That duplication is less severe than the stale-head suppression, but it makes the count and rendered claim links misleading.
Validation I ran:
python -m pytest tests/test_review_bounty_candidates.py -q->9 passed in 0.27spython scripts/review_bounty_candidates.py --repo ramimbo/mergework --bounty-issue 654 --reviewer prettyboyvic --format markdown-> live report reproduced the stale-claim suppression for #841python -m ruff check scripts/review_bounty_candidates.py tests/test_review_bounty_candidates.py-> passedpython -m ruff format --check scripts/review_bounty_candidates.py tests/test_review_bounty_candidates.py->2 files already formattedpython -m mypy scripts/review_bounty_candidates.py-> passedpython scripts/docs_smoke.py->docs smoke okpython -m py_compile scripts/review_bounty_candidates.py tests/test_review_bounty_candidates.py-> passedgit diff --check origin/main...HEAD-> cleangit merge-tree --write-tree origin/main HEAD-> clean tree469b72ad6e8c679489611ed81d689d4c2f52b006
GitHub state checked before review: mergeStateStatus=CLEAN; hosted Quality, readiness, docs, and image checks successful; CodeRabbit successful.
No private data, wallet material, payout execution, treasury mutation, exchange, bridge, cash-out, or price behavior was used.
|
Thanks, this was a valid blocker. Addressed in What changed:
Regression proof:
Validation run locally:
Hosted status checked after push:
No private data, secrets, wallet material, payout execution, treasury mutation, exchange, bridge, cash-out, or price behavior was used. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/review_bounty_candidates.py (1)
188-205:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid reusing one body-level SHA across every PR in a multi-PR claim comment.
Line 203 and Line 204 parse the first
head_sha/base_shafrom the entire comment, butextract_bounty_claims()can emit multiple claims from that same body. A single/claimcomment that listsPR#11and `PR `#12with different SHAs will currently stamp both records with the first SHA it finds, which can misclassify the later PR asalready_claimed_current_headorclaimed_stale_head_or_base.If multi-PR comments are allowed, the SHA/base parsing needs to be scoped per PR reference. If they are not, the safer fallback here is to only attach
head_sha/base_shawhen the comment resolves to exactly one PR; otherwise leave them unset so the claim stays unqualified instead of being attributed to the wrong PR.Also applies to: 208-246
tests/test_review_bounty_candidates.py (1)
194-318:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a base-only claim case; that new branch is still unproven.
The production change now has dedicated
base_claimshandling, but this test only covers stale-head-only and stale-head-plus-base inputs. Ifbase_claimsstopped populating entirely, these assertions would still pass because PR#12already trips the stale-head path. Please add a fixture withbase_shaand nohead_shaand assert the clean/dirty outcomes for that branch.As per coding guidelines, "Add or update tests for changed behavior" and, for
tests/**/*.py, include "negative, replay, boundary, or regression cases where relevant."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 207bb64b-1ebd-4325-81b5-40005a99183a
📒 Files selected for processing (2)
scripts/review_bounty_candidates.pytests/test_review_bounty_candidates.py
caozhengming
left a comment
caozhengming
left a comment
There was a problem hiding this comment.
Requesting changes on current head 1d452b7cd5c0df7e4c811700b5e61db5816f25c9.
I rechecked the latest fix for stale bounty-claim handling. The previous stale-head suppression issue is addressed, and the branch is otherwise healthy, but there is still one correctness blocker in the new claim extraction path.
extract_bounty_claims() can emit more than one claim from a single bounty issue comment: one per PR URL or bare PR #... reference. Each emitted claim then calls _claim_from_comment(), which reads head_sha and base_sha from the entire comment body using the first regex match. That means a multi-PR /claim comment with one Head SHA: line, or multiple PR sections with different SHAs, will stamp every extracted PR claim with the first body-level SHA. Downstream classification can then mark the wrong PR as already_claimed_current_head or claimed_stale_head_or_base.
This matters for the exact saturation workflow this helper is trying to protect: the active review-bounty issue comments are not guaranteed to be one PR per comment, and a wrong SHA attribution can suppress a clean current head from review or incorrectly classify it as already saturated. A safer behavior would be to attach head_sha/base_sha only when the comment resolves to exactly one PR, or to parse SHA evidence from a PR-scoped fragment. Otherwise, leave those fields unset so the claim is treated as unqualified rather than attributed to the wrong PR.
Validation I ran on this head:
uv run --extra dev pytest tests/test_review_bounty_candidates.py -q-> 9 passedruff check scripts/review_bounty_candidates.py tests/test_review_bounty_candidates.py-> passedruff format --check scripts/review_bounty_candidates.py tests/test_review_bounty_candidates.py-> 2 files already formattedmypy scripts/review_bounty_candidates.py-> successpython scripts/docs_smoke.py-> docs smoke okgit diff --check origin/main...HEAD-> cleangit merge-tree --write-tree origin/main HEAD-> clean tree7044f1225cdb6f07f40ac45544b6af180757ed77- GitHub reports
mergeStateStatus=CLEAN; hosted Quality/readiness/docs/image and CodeRabbit are success on this head
No private data, wallet material, payout execution, treasury mutation, exchange, bridge, cash-out, or price behavior was used.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/review_bounty_candidates.py (2)
225-242:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the strongest evidence type per PR before deduping.
seenis keyed only by(pull_request, comment_key), so the first/pull/<n>URL in a comment wins even if a later link for the same PR is#issuecomment-...or#pullrequestreview-.... That silently downgradesevidence_typeand can flip classification fromclaimed_by_pr_commenttoalready_claimed_on_bounty_issue.Proposed fix
def extract_bounty_claims(comments: list[dict[str, Any]]) -> list[dict[str, Any]]: claims: list[dict[str, Any]] = [] seen: set[tuple[int, str | None]] = set() + evidence_rank = {"pr_reference": 0, "pr_review": 1, "pr_comment": 2} for comment_index, comment in enumerate(comments): body = str(comment.get("body") or "") if "/claim" not in body.lower(): continue comment_url = _comment_url(comment) comment_key = comment_url or f"comment-{comment_index}" @@ - for match in PR_URL_RE.finditer(body): + best_url_match_by_pr: dict[int, tuple[str, str]] = {} + for match in PR_URL_RE.finditer(body): pull_request = int(match.group("number")) evidence_url = match.group(0) evidence_type = _claim_evidence_type(match.group("fragment"), body) + current = best_url_match_by_pr.get(pull_request) + if current is None or evidence_rank[evidence_type] > evidence_rank[current[1]]: + best_url_match_by_pr[pull_request] = (evidence_url, evidence_type) + + for pull_request, (evidence_url, evidence_type) in best_url_match_by_pr.items(): url_key = (pull_request, comment_key) if url_key in seen: continue seen.add(url_key) claims.append(
476-484:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFilter missing URLs before truncating markdown claim links.
This slices
bounty_claimsbefore dropping entries withouturl. If the first two claim summaries are URL-less, the markdown report omits later valid claim links for the same PR, while the text report still shows them.Proposed fix
- claim_links = [ - f"[claim]({claim['url']})" - for claim in bounty_claims[:2] - if isinstance(claim, dict) and claim.get("url") - ] + claim_links = [ + f"[claim]({claim['url']})" + for claim in bounty_claims + if isinstance(claim, dict) and claim.get("url") + ][:2]
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 17374b33-d8ca-4eab-a7b8-942936870480
📒 Files selected for processing (2)
scripts/review_bounty_candidates.pytests/test_review_bounty_candidates.py
|
Thanks, this is a valid correctness issue. Addressed in What changed:
Direct reproduction after the patch:
Validation run locally:
Hosted status checked after push:
No private data, secrets, wallet material, payout execution, treasury mutation, exchange, bridge, cash-out, or price behavior was used. |
|
Update after the hosted queue finished: current head |
|
Maintainer queue status: marked needs-info. A newer head landed after the prior human changes-requested review, so this needs current-head human review confirming the claim-thread SHA attribution issues are resolved before merge/payment. |
…-claim-saturation-cycle137 # Conflicts: # scripts/review_bounty_candidates.py
|
Mergeability update for current head
Local verification:
Hosted status at the time of this comment: Quality/readiness/docs/image is |
|
Follow-up on current head
The local verification from the merge-sync remains:
Ready for current-head human review on the SHA-attribution fixes. |
heickerv1001-dev
left a comment
heickerv1001-dev
left a comment
There was a problem hiding this comment.
Current-head review for 53c539347f2397ba1be7a8ceeaab25c7e25b07b5.
I reviewed the latest merge-sync head after the earlier stale-claim and multi-PR SHA-attribution blockers. I do not see a remaining blocker in the claim-thread classification path.
Evidence checked:
- GitHub PR metadata now reports
mergeable=true,mergeable_state=cleanon head53c539347f2397ba1be7a8ceeaab25c7e25b07b5. - Hosted
Quality, readiness, docs, and image checkscheck-run completed withconclusion=successfor the same head. - Commit status API reports
CodeRabbit=successfor the same head. - Inspected the changed surfaces:
scripts/review_bounty_candidates.py,scripts/proposed_work_triage.py, andtests/test_review_bounty_candidates.py. - Verified the script still loads with the new CLI option:
python scripts/review_bounty_candidates.py --helpshows--bounty-issueand exits 0. py_compilepasses forscripts/review_bounty_candidates.py,scripts/proposed_work_triage.py, andtests/test_review_bounty_candidates.py.- Ran direct function-level reproductions for the previously reported edge cases:
- abbreviated current-head claim SHA is classified as
already_claimed_current_head; - uppercase current-head claim SHA is classified as
already_claimed_current_head; - clean PR with stale-head claim remains
candidate_for_fresh_review; - clean PR with base-only claim remains
candidate_for_fresh_review; - dirty PR with base-only/stale evidence is still held as
claimed_stale_head_or_base; - multi-PR claim comments no longer stamp the body-level
Head SHA/Base SHAonto every extracted PR claim; the multi-PR claim remains unqualified withhead_sha=Noneandbase_sha=None.
- abbreviated current-head claim SHA is classified as
Local command evidence from the direct reproduction:
{
"abbrev_current_head": "already_claimed_current_head",
"uppercase_current_head": "already_claimed_current_head",
"clean_stale_head_followup": "candidate_for_fresh_review",
"clean_base_only_followup": "candidate_for_fresh_review",
"multi_pr_sha_unqualified": {
"state": "already_claimed_on_bounty_issue",
"head_sha": null,
"base_sha": null
},
"dirty_base_only_blocked": "claimed_stale_head_or_base"
}
I could not run the repository pytest suite in this local sandbox because this runtime does not have pytest installed, so I did not rely on that. The focused direct reproductions above cover the current-head maintainer ask: confirming the previously reported SHA attribution issues are resolved on the latest head.
No private data, secrets, wallet material, payout execution, treasury mutation, exchange, bridge, cash-out, or price behavior was used.
xiefuzheng713-alt
left a comment
xiefuzheng713-alt
left a comment
There was a problem hiding this comment.
Reviewed current head 53c539347f2397ba1be7a8ceeaab25c7e25b07b5 as a non-author current-head review.
I did not find a remaining code blocker in the claim-thread saturation logic on this head. The earlier SHA-prefix/case, stale-head suppression, base-only, and multi-PR body-level SHA attribution issues are covered by the current test suite and by the live read-only smoke path. Caveat: the PR still has the mrwk:needs-info label, so this review is only a current-head technical review; maintainers still need to decide whether that label is satisfied.
Evidence checked:
- inspected
scripts/review_bounty_candidates.py,scripts/proposed_work_triage.py, andtests/test_review_bounty_candidates.py; - confirmed the CLI exposes
--bounty-issueonly for live--repomode and still supports deterministic fixture mode; - confirmed tests cover abbreviated/uppercase current-head claim SHAs, stale-head claims on clean PRs, base-only claim handling, multi-PR claim comments that must not inherit body-level SHAs, and markdown claim-link rendering;
- live read-only smoke against bounty issue #838 succeeded:
review_bounty_candidates.py --repo ramimbo/mergework --bounty-issue 838 --reviewer xiefuzheng713-alt --format jsonreturned 31 PR rows and classified PR #841 asneeds_infobecause of the existing label, while keeping my already-reviewed PRs out of fresh-review states; - no GitHub mutation, issue edit, label edit, bounty creation, payout, proof, wallet, treasury, or ledger path is invoked by the script.
Validation I ran:
uv run --extra dev pytest tests/test_review_bounty_candidates.py tests/test_proposed_work_triage.py -q-> 34 passeduv run --extra dev pytest -q-> 793 passed, 1 warninguv run --extra dev ruff check scripts/review_bounty_candidates.py scripts/proposed_work_triage.py tests/test_review_bounty_candidates.py tests/test_proposed_work_triage.py-> passeduv run --extra dev ruff format --check scripts/review_bounty_candidates.py scripts/proposed_work_triage.py tests/test_review_bounty_candidates.py tests/test_proposed_work_triage.py-> 4 files already formatteduv run --extra dev mypy scripts/review_bounty_candidates.py scripts/proposed_work_triage.py-> successuv run --extra dev python scripts/docs_smoke.py-> docs smoke okgit diff --check origin/main...HEAD-> cleangit merge-tree --write-tree origin/main HEAD-> clean tree219e2d15ebbaee640179da345760ae761c47c6e2
GitHub state checked: mergeStateStatus=CLEAN; hosted Quality/readiness/docs/image and CodeRabbit checks are successful on this head. No private data, secrets, wallet material, payout execution, treasury mutation, ledger mutation, exchange, bridge, cash-out, MRWK price behavior, or issue mutation was used.
7 participants
Summary
scripts/review_bounty_candidates.py./claimcomments from a live bounty issue into the reviewer candidate report.already_claimed_current_head,already_claimed_on_bounty_issue,claimed_by_pr_comment, orclaimed_stale_head_or_basewhen the active bounty thread already contains relevant evidence.--bounty-issuemode using publicgh api --paginate --slurpissue-comment reads.Bounty #799
Source report: #797
Why this matches #797
#797 asks the review candidate helper to stop relying only on PR review objects and to read active review-bounty issue comments so reviewers can avoid duplicate or stale review claims. This PR implements that focused first slice in the existing helper without posting claims, mutating issues, editing labels, or touching payment state.
Verification
uv run --extra dev pytest tests/test_review_bounty_candidates.py -q-> 8 passeduv run --extra dev pytest -q-> 751 passed, 1 Starlette/httpx deprecation warninguv run --extra dev ruff format --check scripts/review_bounty_candidates.py tests/test_review_bounty_candidates.py-> 2 files already formatteduv run --extra dev ruff check scripts/review_bounty_candidates.py tests/test_review_bounty_candidates.py-> All checks passeduv run --extra dev mypy scripts/review_bounty_candidates.py-> Successgit diff --check-> cleanLive read-only smoke
Ran:
Observed current public claim-thread signal joined into the report:
Safety
Read-only GitHub public reads only. No automatic claim submission, issue editing, labels, acceptance decisions, payments, admin-token APIs, private data, secrets, or wallet material.
Summary by CodeRabbit
New Features
Bug Fixes
Tests