Guard PR bodies against premature payment-status wording#1177
Guard PR bodies against premature payment-status wording#1177yanyishuai wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 53 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 (7)
📝 WalkthroughWalkthroughAdds payment/status wording detection, integrates it into submission evaluation and pull-request CI, and documents the rule. Tests cover detection, reporting, gate outcomes, and live-context behavior. ChangesPayment Language Guard
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 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: 4ac2489e-7f1f-4882-9747-4afb086748a8
📒 Files selected for processing (5)
scripts/check_pr_payment_language.pyscripts/public_payment_language.pyscripts/submission_quality_gate.pytests/test_public_payment_language.pytests/test_submission_quality_gate.py
| pr = _load_pull_request(args.repo, args.pr) | ||
| text = "\n".join(str(pr.get(key) or "") for key in ("title", "body")) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Check only the PR body in GitHub mode.
This CLI is described as a PR-body checker, but this path concatenates the title and body. A compliant PR body will still fail if the title says something like ban "paid" wording, which changes the gate's contract from the one described in this PR.
Suggested fix
- text = "\n".join(str(pr.get(key) or "") for key in ("title", "body"))
+ text = str(pr.get("body") or "")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pr = _load_pull_request(args.repo, args.pr) | |
| text = "\n".join(str(pr.get(key) or "") for key in ("title", "body")) | |
| pr = _load_pull_request(args.repo, args.pr) | |
| text = str(pr.get("body") or "") |
qingfeng312
left a comment
qingfeng312
left a comment
There was a problem hiding this comment.
CI is currently failing on the new submission-quality gate test.
The implementation adds the payment_status_language check to evaluate_submission, so the result now contains one additional check entry. tests/test_submission_quality_gate.py::test_submission_quality_gate_passes_open_bounty_with_evidence still asserts the old exact set of six checks and fails because the left side includes {"payment_status_language": "pass"}.
Please update the expected check set in that test, and any related fixed-shape assertions, so the new payment-status-language guard is covered by the passing path.
Evidence: GitHub Actions run 28343359265, job 83961953872, 1 failed, 912 passed.
fbde72f to
6d89af2
Compare
|
@qingfeng312 Addressed the failing quality-gate test.
Latest head: |
|
@qingfeng312 CI is green on the latest head ( |
1 similar comment
|
@qingfeng312 CI is green on the latest head ( |
qingfeng312
left a comment
qingfeng312
left a comment
There was a problem hiding this comment.
Follow-up on current head 6d89af2bd6df91cadfbf427a2e0987ae74ec6440.
The earlier quality-gate expectation blocker has been resolved. I rechecked the payment/status-language detector, the standalone PR-body checker, the submission quality gate integration, and the associated tests.
The updated test coverage now includes the new payment_status_language pass case, and the guard remains focused on reserved payment/status wording while allowing neutral lifecycle language.
Validation checked: GitHub CI Quality, readiness, docs, and image checks passed on run 28349183723; CodeRabbit status is success on this head. I did not find a remaining blocker for the scoped #1107 change.
|
@qingfeng312 Thanks again for the approval on #1107 — the payment-language guard remains green on Merge-ready whenever convenient. |
|
@qingfeng312 Follow-up on #1107 — payment-language guard still passes the full quality gate on Merge-ready whenever convenient. |
17 similar comments
|
@qingfeng312 Follow-up on #1107 — payment-language guard still passes the full quality gate on Merge-ready whenever convenient. |
|
@qingfeng312 Follow-up on #1107 — payment-language guard still passes the full quality gate on Merge-ready whenever convenient. |
|
@qingfeng312 Follow-up on #1107 — payment-language guard still passes the full quality gate on Merge-ready whenever convenient. |
|
@qingfeng312 Follow-up on #1107 — payment-language guard still passes the full quality gate on Merge-ready whenever convenient. |
|
@qingfeng312 Follow-up on #1107 — payment-language guard still passes the full quality gate on Merge-ready whenever convenient. |
|
@qingfeng312 Follow-up on #1107 — payment-language guard still passes the full quality gate on Merge-ready whenever convenient. |
|
@qingfeng312 Follow-up on #1107 — payment-language guard still passes the full quality gate on Merge-ready whenever convenient. |
|
@qingfeng312 Follow-up on #1107 — payment-language guard still passes the full quality gate on Merge-ready whenever convenient. |
|
@qingfeng312 Follow-up on #1107 — payment-language guard still passes the full quality gate on Merge-ready whenever convenient. |
|
@qingfeng312 Follow-up on #1107 — payment-language guard still passes the full quality gate on Merge-ready whenever convenient. |
|
@qingfeng312 Follow-up on #1107 — payment-language guard still passes the full quality gate on Merge-ready whenever convenient. |
|
@qingfeng312 Follow-up on #1107 — payment-language guard still passes the full quality gate on Merge-ready whenever convenient. |
|
@qingfeng312 Follow-up on #1107 — payment-language guard still passes the full quality gate on Merge-ready whenever convenient. |
|
@qingfeng312 Follow-up on #1107 — payment-language guard still passes the full quality gate on Merge-ready whenever convenient. |
|
@qingfeng312 Follow-up on #1107 — payment-language guard still passes the full quality gate on Merge-ready whenever convenient. |
|
@qingfeng312 Follow-up on #1107 — payment-language guard still passes the full quality gate on Merge-ready whenever convenient. |
|
@qingfeng312 Follow-up on #1107 — payment-language guard still passes the full quality gate on Merge-ready whenever convenient. |
|
@qingfeng312 CI is green on the latest head ( |
|
@qingfeng312 Follow-up on #1107 — payment-language guard still passes the full quality gate on Merge-ready whenever convenient. |
6109fc8 to
979be52
Compare
|
@qingfeng312 — addressed the review note on the duplicated detector: |
979be52 to
cabe127
Compare
|
@qingfeng312 — fixed the heading detector so |
cabe127 to
77e25a9
Compare
77e25a9 to
192ddfb
Compare
yanyishuai
changed the title
192ddfb to
35a1895
Compare
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: 63e1590c-6661-4abd-be20-28df01b72b1b
📒 Files selected for processing (7)
.github/workflows/ci.ymlAGENTS.mdscripts/check_pr_payment_language.pyscripts/public_payment_language.pyscripts/submission_quality_gate.pytests/test_public_payment_language.pytests/test_submission_quality_gate.py
| text = "\n".join(str(pr.get(key) or "") for key in ("title", "body")) | ||
| context = { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Check only the PR body in GitHub mode.
This CLI is described as a PR-body checker, but this path concatenates the title and body. A compliant PR body will still fail if the title contains restricted wording (like ban "paid" wording), which changes the gate's contract from the one described.
Proposed fix
- text = "\n".join(str(pr.get(key) or "") for key in ("title", "body"))
+ text = str(pr.get("body") or "")| for line in text.splitlines(): | ||
| content = _content_line(line) | ||
| if not content: | ||
| continue | ||
| if _line_is_allowlisted(line) or _line_is_allowlisted(content): | ||
| continue | ||
| if _PAYOUT_BOUNDARY_RE.search(content): | ||
| violations.append( | ||
| "deprecated 'Payout boundary' heading found; prefer neutral " | ||
| "'Submission status' wording" | ||
| ) | ||
| break | ||
| if _LEGACY_WITHDRAWABLE_RE.search(content): | ||
| violations.append( | ||
| "legacy 'not confirmed or withdrawable' phrasing found; " | ||
| "use neutral submission status language" | ||
| ) | ||
| break | ||
|
|
||
| for line in text.splitlines(): | ||
| content = _content_line(line) | ||
| if not content: | ||
| continue | ||
| if _line_is_allowlisted(line) or _line_is_allowlisted(content): | ||
| continue | ||
| for pattern in _RESERVED_STATUS_ASSERTION_RES: | ||
| if pattern.search(content): | ||
| violations.append( | ||
| f"reserved payment/status wording used as a claim assertion: {content[:120]}" | ||
| ) | ||
| break |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Merge redundant loops and remove premature break statements.
The first for line in text.splitlines(): loop uses break when it finds a match for _PAYOUT_BOUNDARY_RE or _LEGACY_WITHDRAWABLE_RE. This immediately exits the line loop, meaning any subsequent lines in the text are never checked for these patterns.
Consolidating these checks into a single loop without those premature breaks ensures all violations are found in a single pass. (The break inside the _RESERVED_STATUS_ASSERTION_RES loop is correct because it only breaks the inner pattern loop).
Proposed fix
- for line in text.splitlines():
- content = _content_line(line)
- if not content:
- continue
- if _line_is_allowlisted(line) or _line_is_allowlisted(content):
- continue
- if _PAYOUT_BOUNDARY_RE.search(content):
- violations.append(
- "deprecated 'Payout boundary' heading found; prefer neutral "
- "'Submission status' wording"
- )
- break
- if _LEGACY_WITHDRAWABLE_RE.search(content):
- violations.append(
- "legacy 'not confirmed or withdrawable' phrasing found; "
- "use neutral submission status language"
- )
- break
-
- for line in text.splitlines():
+ for line in text.splitlines():
content = _content_line(line)
if not content:
continue
if _line_is_allowlisted(line) or _line_is_allowlisted(content):
continue
+ if _PAYOUT_BOUNDARY_RE.search(content):
+ violations.append(
+ "deprecated 'Payout boundary' heading found; prefer neutral "
+ "'Submission status' wording"
+ )
+ if _LEGACY_WITHDRAWABLE_RE.search(content):
+ violations.append(
+ "legacy 'not confirmed or withdrawable' phrasing found; "
+ "use neutral submission status language"
+ )
for pattern in _RESERVED_STATUS_ASSERTION_RES:
if pattern.search(content):
violations.append(
f"reserved payment/status wording used as a claim assertion: {content[:120]}"
)
break| violations = find_payment_language_violations("This submission is paid.") | ||
| assert any("reserved payment/status wording" in item for item in violations) | ||
|
|
||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a test for multiple violations in a single text.
There is currently no test verifying that the detector can find multiple distinct violations within the same text. As per path instructions, include regression cases where relevant. Adding this case ensures that early exits (like the break bug in the detector) do not silently mask subsequent findings.
Proposed fix
+def test_find_payment_language_flags_multiple_violations() -> None:
+ violations = find_payment_language_violations(
+ "## Payout boundary\n"
+ "This reward is not confirmed or withdrawable yet.\n"
+ "This submission is paid."
+ )
+ assert len(violations) == 3
+ assert any("Payout boundary" in item for item in violations)
+ assert any("legacy" in item for item in violations)
+ assert any("reserved payment/status wording" in item for item in violations)
+
+📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_find_payment_language_flags_multiple_violations() -> None: | |
| violations = find_payment_language_violations( | |
| "## Payout boundary\n" | |
| "This reward is not confirmed or withdrawable yet.\n" | |
| "This submission is paid." | |
| ) | |
| assert len(violations) == 3 | |
| assert any("Payout boundary" in item for item in violations) | |
| assert any("legacy" in item for item in violations) | |
| assert any("reserved payment/status wording" in item for item in violations) |
Source: Path instructions
|
@qingfeng312 — CI fully green on latest head (synced from #1168 / bounty #1107). Current head ( Please recheck when convenient. Wallet: |
|
@qingfeng312 — current head is CI green after the latest review fixes. Ready for another look when convenient. |
|
@qingfeng312 Follow-up on #1107 — payment-language guard still passes the full quality gate on Merge-ready whenever convenient. |
|
@qingfeng312 — CI fully green on latest head (synced from #1168 / bounty #1107). Current head ( Please recheck when convenient. Wallet: |
|
@qingfeng312 — please recheck current head 35a1895. |
|
@qingfeng312 — CI fully green on latest head (synced from #1168 / bounty #1107). Current head ( Please recheck when convenient. Wallet: |
3 similar comments
|
@qingfeng312 — CI fully green on latest head (synced from #1168 / bounty #1107). Current head ( Please recheck when convenient. Wallet: |
|
@qingfeng312 — CI fully green on latest head (synced from #1168 / bounty #1107). Current head ( Please recheck when convenient. Wallet: |
|
@qingfeng312 — CI fully green on latest head (synced from #1168 / bounty #1107). Current head ( Please recheck when convenient. Wallet: |
Re-review request (current head
|
|
@qingfeng312 Follow-up on #1107 — payment-language guard still passes the full quality gate on Merge-ready whenever convenient. |
CI green re-review@qingfeng312 — synced payment-language guard is CI success / mergeable=clean (prior CR on older SHA). Please recheck when convenient. Wallet: |
Same #1107 scope as #1168@qingfeng312 — current head To reduce queue noise: PR #1168 covers the same payment-language guard with CI green + @piaigmt APPROVED. Happy to treat #1168 as the merge candidate and close this twin if maintainers prefer one PR. Wallet: |
|
@qingfeng312 Follow-up on #1107 — payment-language guard still passes the full quality gate on Merge-ready whenever convenient. |
|
@qingfeng312 CI is green on the latest head ( |
2 participants
Summary
Adds a bounded public payment/status-language guard for MergeWork submission drafts and PR bodies.
Why
Docs already reserve payment/status words for proof-backed states, but the quality gate did not enforce that rule. This closes the manual review gap for open PRs like #1072/#1075/#1077.
Tests
�ash python -m pytest tests/test_public_payment_language.py -q python -m pytest tests/test_submission_quality_gate.py -q -k paymentWallet
Do4v7foHJvRJLpRRoGaVPWX6DDEjX3yTK7J91gpwUQpE
Related to #1107
Summary by CodeRabbit