Unify live GitHub collection safety-cap policy across report scripts#1165
Unify live GitHub collection safety-cap policy across report scripts#1165yanyishuai wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 35 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 (6)
📝 WalkthroughWalkthroughAdds a shared ChangesShared safety-cap constants and script reformatting
Bounty saturation classifier expansion
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (2 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: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 99e997bf-9ef0-4f4e-956b-343a96c32fa9
📒 Files selected for processing (7)
scripts/check_live_bounty_closing_refs.pyscripts/gh_collection_caps.pyscripts/pr_queue_health.pyscripts/review_bounty_candidates.pyscripts/submission_quality_gate.pytests/test_gh_collection_caps.pytests/test_pr_queue_health.py
| def load_live_data(repo: str, api_host: str, state: str, pr_numbers: list[int]) -> dict[str, Any]: | ||
| return { | ||
| "bounties": load_public_bounty_list(api_host, query="status=open&limit=200"), | ||
| "pull_requests": _load_pull_requests(repo, state, pr_numbers), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Filter the public bounty query by repo.
analyze_closing_refs() only matches on issue numbers, but Line 164 now loads every open bounty from the public API. GitHub issue numbers are repo-scoped, so an open bounty #123 in another repo can collide with this repo’s #123 and trigger a false violation; the global 200-row cap can also crowd out this repo’s own bounties.
Suggested fix
def load_live_data(repo: str, api_host: str, state: str, pr_numbers: list[int]) -> dict[str, Any]:
return {
- "bounties": load_public_bounty_list(api_host, query="status=open&limit=200"),
+ "bounties": load_public_bounty_list(
+ api_host,
+ query=f"status=open&limit=200&repo={repo}",
+ ),
"pull_requests": _load_pull_requests(repo, state, pr_numbers),
}📝 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 load_live_data(repo: str, api_host: str, state: str, pr_numbers: list[int]) -> dict[str, Any]: | |
| return { | |
| "bounties": load_public_bounty_list(api_host, query="status=open&limit=200"), | |
| "pull_requests": _load_pull_requests(repo, state, pr_numbers), | |
| def load_live_data(repo: str, api_host: str, state: str, pr_numbers: list[int]) -> dict[str, Any]: | |
| return { | |
| "bounties": load_public_bounty_list( | |
| api_host, | |
| query=f"status=open&limit=200&repo={repo}", | |
| ), | |
| "pull_requests": _load_pull_requests(repo, state, pr_numbers), |
| mode: str = FAIL_ON_SATURATION, | ||
| issue_warn_detail: str | None = None, | ||
| ) -> str | None: | ||
| if len(items) < cap: | ||
| return None | ||
| message = saturation_message( | ||
| collection=collection, | ||
| cap=cap, | ||
| warn_only=(mode == WARN_ON_SATURATION), | ||
| issue_warn_detail=issue_warn_detail, | ||
| ) | ||
| if mode == FAIL_ON_SATURATION: | ||
| raise RuntimeError(message) | ||
| return message |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject unsupported saturation modes explicitly.
Any value other than "fail" skips the exception path here. A typo silently downgrades fail-fast callers into warn-only behavior and can let saturated gh output be treated as trustworthy. Validate mode up front and raise ValueError for unknown values.
Proposed fix
def check_collection_saturation(
items: list[Any],
*,
cap: int,
collection: str,
mode: str = FAIL_ON_SATURATION,
issue_warn_detail: str | None = None,
) -> str | None:
+ if mode not in {FAIL_ON_SATURATION, WARN_ON_SATURATION}:
+ raise ValueError(f"unsupported saturation mode: {mode}")
if len(items) < cap:
return None
message = saturation_message(
collection=collection,
cap=cap,📝 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.
| mode: str = FAIL_ON_SATURATION, | |
| issue_warn_detail: str | None = None, | |
| ) -> str | None: | |
| if len(items) < cap: | |
| return None | |
| message = saturation_message( | |
| collection=collection, | |
| cap=cap, | |
| warn_only=(mode == WARN_ON_SATURATION), | |
| issue_warn_detail=issue_warn_detail, | |
| ) | |
| if mode == FAIL_ON_SATURATION: | |
| raise RuntimeError(message) | |
| return message | |
| mode: str = FAIL_ON_SATURATION, | |
| issue_warn_detail: str | None = None, | |
| ) -> str | None: | |
| if mode not in {FAIL_ON_SATURATION, WARN_ON_SATURATION}: | |
| raise ValueError(f"unsupported saturation mode: {mode}") | |
| if len(items) < cap: | |
| return None | |
| message = saturation_message( | |
| collection=collection, | |
| cap=cap, | |
| warn_only=(mode == WARN_ON_SATURATION), | |
| issue_warn_detail=issue_warn_detail, | |
| ) | |
| if mode == FAIL_ON_SATURATION: | |
| raise RuntimeError(message) | |
| return message |
| def test_check_collection_saturation_passes_below_cap() -> None: | ||
| caps.check_collection_saturation([1, 2], cap=3, collection="pr") | ||
|
|
||
|
|
||
| def test_check_collection_saturation_fails_at_cap() -> None: | ||
| with pytest.raises(RuntimeError, match="pr list reached the 2 item safety cap"): | ||
| caps.check_collection_saturation([1, 2], cap=2, collection="pr") | ||
|
|
||
|
|
||
| def test_check_collection_saturation_warn_mode_returns_message() -> None: | ||
| message = caps.check_collection_saturation( | ||
| [1, 2], | ||
| cap=2, | ||
| collection="issue", | ||
| mode=caps.WARN_ON_SATURATION, | ||
| ) | ||
| assert message is not None | ||
| assert "referenced-issue checks may be incomplete" in message |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover the full helper contract in the regression tests.
The new suite never asserts the below-cap None return, and it does not exercise the issue_warn_detail override that scripts/submission_quality_gate.py:682-688 now depends on. A regression in either branch would still pass here.
Proposed test additions
def test_check_collection_saturation_passes_below_cap() -> None:
- caps.check_collection_saturation([1, 2], cap=3, collection="pr")
+ assert caps.check_collection_saturation([1, 2], cap=3, collection="pr") is None
@@
def test_check_collection_saturation_warn_mode_returns_message() -> None:
message = caps.check_collection_saturation(
[1, 2],
cap=2,
collection="issue",
mode=caps.WARN_ON_SATURATION,
)
assert message is not None
assert "referenced-issue checks may be incomplete" in message
+
+
+def test_check_collection_saturation_warn_mode_uses_custom_issue_detail() -> None:
+ message = caps.check_collection_saturation(
+ [1, 2],
+ cap=2,
+ collection="issue",
+ mode=caps.WARN_ON_SATURATION,
+ issue_warn_detail="bounty discovery may be incomplete",
+ )
+ assert (
+ message
+ == "gh issue list reached the 2 item safety cap; "
+ "bounty discovery may be incomplete"
+ )📝 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_check_collection_saturation_passes_below_cap() -> None: | |
| caps.check_collection_saturation([1, 2], cap=3, collection="pr") | |
| def test_check_collection_saturation_fails_at_cap() -> None: | |
| with pytest.raises(RuntimeError, match="pr list reached the 2 item safety cap"): | |
| caps.check_collection_saturation([1, 2], cap=2, collection="pr") | |
| def test_check_collection_saturation_warn_mode_returns_message() -> None: | |
| message = caps.check_collection_saturation( | |
| [1, 2], | |
| cap=2, | |
| collection="issue", | |
| mode=caps.WARN_ON_SATURATION, | |
| ) | |
| assert message is not None | |
| assert "referenced-issue checks may be incomplete" in message | |
| def test_check_collection_saturation_passes_below_cap() -> None: | |
| assert caps.check_collection_saturation([1, 2], cap=3, collection="pr") is None | |
| def test_check_collection_saturation_fails_at_cap() -> None: | |
| with pytest.raises(RuntimeError, match="pr list reached the 2 item safety cap"): | |
| caps.check_collection_saturation([1, 2], cap=2, collection="pr") | |
| def test_check_collection_saturation_warn_mode_returns_message() -> None: | |
| message = caps.check_collection_saturation( | |
| [1, 2], | |
| cap=2, | |
| collection="issue", | |
| mode=caps.WARN_ON_SATURATION, | |
| ) | |
| assert message is not None | |
| assert "referenced-issue checks may be incomplete" in message | |
| def test_check_collection_saturation_warn_mode_uses_custom_issue_detail() -> None: | |
| message = caps.check_collection_saturation( | |
| [1, 2], | |
| cap=2, | |
| collection="issue", | |
| mode=caps.WARN_ON_SATURATION, | |
| issue_warn_detail="bounty discovery may be incomplete", | |
| ) | |
| assert ( | |
| message | |
| == "gh issue list reached the 2 item safety cap; " | |
| "bounty discovery may be incomplete" | |
| ) |
Sources: Coding guidelines, Path instructions
| def test_pr_queue_health_fails_fast_when_issue_fetch_hits_cap(monkeypatch) -> None: | ||
| def fake_run(args, **kwargs): | ||
| if args[:3] == ["gh", "pr", "list"]: | ||
| stdout = "[]" | ||
| elif args[:3] == ["gh", "issue", "list"]: | ||
| stdout = json.dumps( | ||
| [ | ||
| {"number": number, "title": "MRWK bounty: many", "state": "OPEN"} | ||
| for number in range(1, 202) | ||
| ] | ||
| ) | ||
| else: | ||
| raise AssertionError(args) | ||
| return subprocess.CompletedProcess(args=args, returncode=0, stdout=stdout, stderr="") | ||
| monkeypatch.setattr(pr_queue_health.subprocess, "run", fake_run) | ||
| with pytest.raises(RuntimeError, match="issue list reached the 200 item safety cap"): | ||
| pr_queue_health.load_live_queue("ramimbo/mergework") | ||
| def test_pr_queue_health_fails_fast_when_pr_fetch_hits_cap(monkeypatch) -> None: | ||
| def fake_run(args, **kwargs): | ||
| if args[:3] == ["gh", "pr", "list"]: | ||
| stdout = json.dumps( | ||
| [ | ||
| { | ||
| "number": number, | ||
| "title": "Open PR", | ||
| "body": "Refs #1", | ||
| "labels": [], | ||
| "mergeStateStatus": "clean", | ||
| } | ||
| for number in range(1, 202) | ||
| ] | ||
| ) | ||
| elif args[:3] == ["gh", "issue", "list"]: | ||
| stdout = "[]" | ||
| else: | ||
| raise AssertionError(args) | ||
| return subprocess.CompletedProcess(args=args, returncode=0, stdout=stdout, stderr="") | ||
| monkeypatch.setattr(pr_queue_health.subprocess, "run", fake_run) | ||
| with pytest.raises(RuntimeError, match="pr list reached the 200 item safety cap"): | ||
| pr_queue_health.load_live_queue("ramimbo/mergework") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the requested gh --limit in these saturation regressions.
Both fakes always return 201 rows, so these tests only prove the updated error wording. They would still pass if load_live_queue() stopped sending --limit 200, which is part of this change. Capture the command args and assert the list call used --limit 200, or make the fake honor the requested limit so the boundary wiring is actually covered. As per path instructions, "Do not request docstrings. Focus on whether tests prove the changed behavior and include negative, replay, boundary, or regression cases where relevant."
🧰 Tools
🪛 ast-grep (0.44.0)
[info] 507-512: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
[
{"number": number, "title": "MRWK bounty: many", "state": "OPEN"}
for number in range(1, 202)
]
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 526-537: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
[
{
"number": number,
"title": "Open PR",
"body": "Refs #1",
"labels": [],
"mergeStateStatus": "clean",
}
for number in range(1, 202)
]
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
Source: Path instructions
qingfeng312
left a comment
qingfeng312
left a comment
There was a problem hiding this comment.
Reviewed current head 2bf1f7822ac5d120404ba867f2402fecd7da98b2.
This branch is not merge-ready because the required CI gate fails before the suite can run. In CI run 28320025428, pytest collection reports three import errors:
tests/test_check_live_bounty_closing_refs.py->ModuleNotFoundError: No module named 'scripts.gh_cli'tests/test_pr_queue_health.py->ModuleNotFoundError: No module named 'scripts.gh_cli'tests/test_review_bounty_candidates.py->ModuleNotFoundError: No module named 'scripts.gh_cli'
The PR introduces scripts/gh_collection_caps.py and updates multiple report scripts to import shared helpers, but the scripts.gh_cli module they now depend on is not part of this branch. A clean checkout therefore cannot collect tests.
Please include the missing shared gh_cli helper or avoid the dependency on it in this PR.
Scope checked: CI log, current PR metadata, CodeRabbit status, and changed-file list only. No wallet, treasury, payout, private data, credentials, or external mutation paths were exercised.
yanyishuai
force-pushed
the
bounty-1141-gh-collection-caps
branch
from
2bf1f78 to
8a834b9
Compare
qingfeng312
left a comment
qingfeng312
left a comment
There was a problem hiding this comment.
Reviewed current head 8a834b9200fe1d3b312af181f2733969b4bbd25c.
This is still not merge-ready. The PR body says it adds scripts/gh_collection_caps.py and tests/test_gh_collection_caps.py, but the current diff contains neither file; the scripts continue to carry local collection cap constants/policy. That means the shared cap-policy abstraction described by #1141 is not actually implemented yet. The hosted quality/readiness check is also failing.
Please add the shared policy module and targeted tests, wire the scripts to that shared module, and reduce the large line-ending/no-op rewrite so the reviewable diff is limited to the intended behavior.
yanyishuai
force-pushed
the
bounty-1141-gh-collection-caps
branch
from
8a834b9 to
68f1fc4
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8bea5397-56a1-47f2-a895-63d1742e360c
📒 Files selected for processing (5)
scripts/check_live_bounty_closing_refs.pyscripts/pr_queue_health.pyscripts/review_bounty_candidates.pyscripts/submission_quality_gate.pytests/test_pr_queue_health.py
| r"https://github\.com/([^/]+)/([^/]+)/pull/(\d+)(?:\b|[^\d])", | ||
| re.IGNORECASE, | ||
| ) | ||
| PR_NUMBER_REF_RE = re.compile(r"\bpull/(\d+)\b|\bPR\s+#(\d+)\b", re.IGNORECASE) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Parse GitHub’s common bare PR shorthand.
/claim #123 or `claiming `#123 passes the claim-signal gate but produces no pr_evidence, because PR_NUMBER_REF_RE only accepts pull/123 and PR #123``. That drops common bounty-claim comments from the saturation index.
Proposed fix
-PR_NUMBER_REF_RE = re.compile(r"\bpull/(\d+)\b|\bPR\s+#(\d+)\b", re.IGNORECASE)
+PR_NUMBER_REF_RE = re.compile(
+ r"\bpull/(\d+)\b|\bPR\s+#(\d+)\b|(?<![\w/])#(\d+)\b",
+ re.IGNORECASE,
+)
...
- pr_str = match.group(1) or match.group(2)
+ pr_str = match.group(1) or match.group(2) or match.group(3)Also applies to: 141-148
| def index_bounty_claims(comments: list[Any], *, repo: str) -> dict[int, list[dict[str, Any]]]: | ||
| by_pr: dict[int, list[dict[str, Any]]] = defaultdict(list) | ||
| for comment in comments: | ||
| if not isinstance(comment, dict): | ||
| continue | ||
| for record in _parse_claim_comment(comment, repo=repo): | ||
| by_pr[int(record["pull_request"])].append(record) | ||
| return dict(by_pr) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make “latest claim” deterministic.
Saturation uses claims[-1], but index_bounty_claims() preserves caller order. If fixture/API comment order changes, stale/current claim decisions can flip. Sort each PR’s claims by submitted_at before returning the index.
Proposed fix
for comment in comments:
if not isinstance(comment, dict):
continue
for record in _parse_claim_comment(comment, repo=repo):
by_pr[int(record["pull_request"])].append(record)
+ for records in by_pr.values():
+ records.sort(key=lambda record: str(record.get("submitted_at") or ""))
return dict(by_pr)Also applies to: 354-354, 384-386
| raw_comments = data.get("bounty_claim_comments") | ||
| saturation_enabled = isinstance(effective_repo, str) and "bounty_claim_comments" in data | ||
| if saturation_enabled and isinstance(raw_comments, list): | ||
| claims_by_pr = index_bounty_claims(raw_comments, repo=effective_repo) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail closed on malformed bounty comments.
When bounty_claim_comments exists but is not a list, saturation remains enabled with zero claims, and dirty PRs can be rewritten as unclaimed. In --bounty-issue mode, malformed claim data should abort instead of changing classifications.
Proposed fix
raw_comments = data.get("bounty_claim_comments")
saturation_enabled = isinstance(effective_repo, str) and "bounty_claim_comments" in data
- if saturation_enabled and isinstance(raw_comments, list):
+ if saturation_enabled and not isinstance(raw_comments, list):
+ raise ValueError("bounty_claim_comments must be a list when present")
+ if saturation_enabled:
claims_by_pr = index_bounty_claims(raw_comments, repo=effective_repo) comments = issue.get("comments", []) if isinstance(issue, dict) else []
if not isinstance(comments, list):
- comments = []
+ raise RuntimeError("gh issue view returned a non-list comments payload")
data["bounty_claim_comments"] = commentsAlso applies to: 582-585
| if effective_repo and saturation_enabled: | ||
| report["bounty_issue_claims_indexed"] = sum(len(items) for items in claims_by_pr.values()) | ||
| return report |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Include indexed-claim totals in rendered reports.
bounty_issue_claims_indexed is added only at the top level, but text/markdown render only report["summary"], so the new saturation total is invisible outside JSON output.
Proposed fix
if effective_repo and saturation_enabled:
- report["bounty_issue_claims_indexed"] = sum(len(items) for items in claims_by_pr.values())
+ report["summary"]["bounty_issue_claims_indexed"] = sum(
+ len(items) for items in claims_by_pr.values()
+ )Also applies to: 471-489
qingfeng312
left a comment
qingfeng312
left a comment
There was a problem hiding this comment.
Reviewed current head 68f1fc4c7a401ea5dd8e9d94d794f26357149c2d.
Requesting changes. The formatting issues are resolved and the hosted quality check is green, but this still does not implement the stated #1141 scope. The PR body says it adds scripts/gh_collection_caps.py and tests/test_gh_collection_caps.py, then normalizes the shared cap policy around that module. The current file list only changes existing maintenance scripts and tests/test_pr_queue_health.py; the shared cap module and its dedicated tests are absent.
Please either add the promised shared cap module/tests or narrow the PR body and implementation so the submitted scope matches what is actually changed.
|
@qingfeng312 — proactive CRLF cleanup on this branch. Normalized LF line endings (no functional changes) in:
Should pass |
yanyishuai
force-pushed
the
bounty-1141-gh-collection-caps
branch
from
68f1fc4 to
c3f3507
Compare
|
@qingfeng312 — added |
yanyishuai
force-pushed
the
bounty-1141-gh-collection-caps
branch
2 times, most recently
from
a9f6d14 to
7632d66
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/check_live_bounty_closing_refs.py (1)
230-264: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo CLI validation added here, despite cohort description.
The cohort summary states
check_live_bounty_closing_refs.py"add[s] CLI argument validation" alongside the other two scripts, butargs.input/args.repoare still used unvalidated here — unlikepr_queue_health.py's new_require_non_empty_argandsubmission_quality_gate.py's new_require_non_empty_path. For parity with the rest of this unification effort, consider adding the same non-empty/whitespace guard before calling_load_input/load_live_data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: af601749-913d-4b03-9d3b-9b7dc4d82b55
📒 Files selected for processing (7)
scripts/check_live_bounty_closing_refs.pyscripts/gh_collection_caps.pyscripts/pr_queue_health.pyscripts/review_bounty_candidates.pyscripts/submission_quality_gate.pytests/test_gh_collection_caps.pytests/test_submission_quality_gate.py
| """Shared GitHub collection safety caps for maintenance scripts.""" | ||
|
|
||
| GH_PR_SAFETY_CAP = 201 | ||
| GH_ISSUE_SAFETY_CAP = 201 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Document the N+1 sentinel semantics.
The constants are 201, but the PR intent (and CLI --limit documentation elsewhere) frames the safety cap as "200 items". This works because callers request --limit str(CAP) and treat len(items) >= CAP as saturation — i.e. 201 is a "200 + 1" sentinel to detect truncation, not the actual configured cap. Since the whole point of this module is to be the single documented source of truth for the cap policy, a bare docstring without this rationale invites future maintainers to "round" the value to 200 and break saturation detection.
📝 Proposed docstring clarification
-"""Shared GitHub collection safety caps for maintenance scripts."""
+"""Shared GitHub collection safety caps for maintenance scripts.
+
+Each cap is the configured safe collection size (200) plus one. Scripts pass
+the cap as `--limit` and treat `len(items) >= CAP` as proof that `gh` output
+was truncated, so lowering these back to 200 would break saturation
+detection.
+"""
GH_PR_SAFETY_CAP = 201
GH_ISSUE_SAFETY_CAP = 201📝 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.
| """Shared GitHub collection safety caps for maintenance scripts.""" | |
| GH_PR_SAFETY_CAP = 201 | |
| GH_ISSUE_SAFETY_CAP = 201 | |
| """Shared GitHub collection safety caps for maintenance scripts. | |
| Each cap is the configured safe collection size (200) plus one. Scripts pass | |
| the cap as `--limit` and treat `len(items) >= CAP` as proof that `gh` output | |
| was truncated, so lowering these back to 200 would break saturation | |
| detection. | |
| """ | |
| GH_PR_SAFETY_CAP = 201 | |
| GH_ISSUE_SAFETY_CAP = 201 |
| from scripts.gh_collection_caps import GH_PR_SAFETY_CAP | ||
|
|
||
| import argparse | ||
| import json | ||
| import re | ||
| import subprocess | ||
| import sys | ||
| from collections import Counter | ||
| from collections import Counter, defaultdict | ||
| from pathlib import Path | ||
| from typing import Any | ||
|
|
||
| DIRTY_MERGE_STATES = {"blocked", "conflicting", "dirty"} | ||
| if __package__ in {None, ""}: | ||
| sys.path.insert(0, str(Path(__file__).resolve().parents[1])) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm scripts is not an importable installed package and repo root is required on sys.path.
fd -a __init__.py scripts 2>/dev/null || echo "no scripts/__init__.py"
rg -n 'packages|py-modules|scripts' pyproject.toml setup.cfg setup.py 2>/dev/null
# Show the import-vs-bootstrap ordering in sibling scripts for consistency.
rg -n 'from scripts.gh_collection_caps|sys.path.insert' scripts/*.pyRepository: ramimbo/mergework
Length of output: 1552
🏁 Script executed:
python3 - <<'PY'
from importlib.machinery import PathFinder
from pathlib import Path
repo = Path.cwd()
script_dir = repo / "scripts"
spec_on_script_dir = PathFinder.find_spec("scripts.gh_collection_caps", [str(script_dir)])
spec_on_repo_root = PathFinder.find_spec("scripts.gh_collection_caps", [str(repo)])
print("repo_root:", repo)
print("script_dir:", script_dir)
print("spec_with_script_dir:", spec_on_script_dir)
print("spec_with_repo_root:", spec_on_repo_root)
PYRepository: ramimbo/mergework
Length of output: 456
Bootstrap sys.path before importing scripts.gh_collection_caps. When this file is run directly, the repo root isn’t on sys.path yet, so the top-level import can fail before the bootstrap runs. Move the bootstrap above the import.
| def _require_non_empty_path( | ||
| parser: argparse.ArgumentParser, option_name: str, value: str | ||
| ) -> str: | ||
| if not value.strip(): | ||
| parser.error(f"{option_name} must be a non-empty path") | ||
| return value |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Validator is less strict than its pr_queue_health.py counterpart.
_require_non_empty_path only rejects empty/whitespace-only values, but pr_queue_health.py's _require_non_empty_arg (added in this same cohort) also rejects values with leading/trailing whitespace. A path like " input.json" would silently pass here and fail deep in open() with a less clear error, while the equivalent --input/--repo value would be rejected upfront in pr_queue_health.py. Consider aligning the two validators (e.g., move a shared version into gh_collection_caps.py) for the "consistent policy across report scripts" goal of this PR.
♻️ Proposed alignment
def _require_non_empty_path(
parser: argparse.ArgumentParser, option_name: str, value: str
) -> str:
- if not value.strip():
+ stripped = value.strip()
+ if not stripped:
parser.error(f"{option_name} must be a non-empty path")
+ if stripped != value:
+ parser.error(f"{option_name} must not include leading or trailing whitespace")
return value📝 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 _require_non_empty_path( | |
| parser: argparse.ArgumentParser, option_name: str, value: str | |
| ) -> str: | |
| if not value.strip(): | |
| parser.error(f"{option_name} must be a non-empty path") | |
| return value | |
| def _require_non_empty_path( | |
| parser: argparse.ArgumentParser, option_name: str, value: str | |
| ) -> str: | |
| stripped = value.strip() | |
| if not stripped: | |
| parser.error(f"{option_name} must be a non-empty path") | |
| if stripped != value: | |
| parser.error(f"{option_name} must not include leading or trailing whitespace") | |
| return value |
| def test_gh_collection_caps_are_positive() -> None: | ||
| assert gcc.GH_PR_SAFETY_CAP >= 1 | ||
| assert gcc.GH_ISSUE_SAFETY_CAP >= 1 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Strengthen the regression assertion.
>= 1 doesn't catch drift back to the old inconsistent values (200/201/101) that #1141 called out. Consider asserting the exact expected value so a future edit that changes the cap without updating this test fails loudly.
✅ Proposed stronger assertions
def test_gh_collection_caps_are_positive() -> None:
- assert gcc.GH_PR_SAFETY_CAP >= 1
- assert gcc.GH_ISSUE_SAFETY_CAP >= 1
+ assert gcc.GH_PR_SAFETY_CAP == 201
+ assert gcc.GH_ISSUE_SAFETY_CAP == 201📝 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_gh_collection_caps_are_positive() -> None: | |
| assert gcc.GH_PR_SAFETY_CAP >= 1 | |
| assert gcc.GH_ISSUE_SAFETY_CAP >= 1 | |
| def test_gh_collection_caps_are_positive() -> None: | |
| assert gcc.GH_PR_SAFETY_CAP == 201 | |
| assert gcc.GH_ISSUE_SAFETY_CAP == 201 |
Source: Path instructions
yanyishuai
force-pushed
the
bounty-1141-gh-collection-caps
branch
2 times, most recently
from
007312a to
6dcd0e9
Compare
yanyishuai
force-pushed
the
bounty-1141-gh-collection-caps
branch
from
6dcd0e9 to
83fe1f6
Compare
yanyishuai
force-pushed
the
bounty-1141-gh-collection-caps
branch
from
83fe1f6 to
1f5c28e
Compare
|
@qingfeng312 — CI fully green on latest head for bounty #1141. Fixes on current head (
Please recheck when convenient. Wallet: |
3 similar comments
|
@qingfeng312 — CI fully green on latest head for bounty #1141. Fixes on current head (
Please recheck when convenient. Wallet: |
|
@qingfeng312 — CI fully green on latest head for bounty #1141. Fixes on current head (
Please recheck when convenient. Wallet: |
|
@qingfeng312 — CI fully green on latest head for bounty #1141. Fixes on current head (
Please recheck when convenient. Wallet: |
taherdhanera
left a comment
taherdhanera
left a comment
There was a problem hiding this comment.
Reviewed current head 1f5c28e7f9d5dd177721126dbb0edd6023396624. This still needs changes before merge.
The current diff only adds scripts/gh_collection_caps.py and tests/test_gh_collection_caps.py. It does not wire the shared caps into the affected scripts from #1141, so the proposed cap-policy unification is not actually applied yet.
Current source evidence on this head:
scripts/pr_queue_health.pystill defines localGH_PR_SAFETY_CAP = 201andGH_ISSUE_SAFETY_CAP = 201.scripts/check_live_bounty_closing_refs.pystill defines localGH_PR_SAFETY_CAP = 200.scripts/review_bounty_candidates.pystill defines localGH_PR_SAFETY_CAP = 201.scripts/submission_quality_gate.pystill defines localGH_PR_SAFETY_CAP = 101andGH_ISSUE_SAFETY_CAP = 201.- None of those affected scripts import
scripts.gh_collection_capson the current head.
That means the inconsistent thresholds called out in #1141 remain in place, and the new test only checks that the constants are positive. It does not cover saturation behavior or prove the scripts use the shared policy. #1141 also has an earlier competing PR #1156 noted on the issue, so this PR should either truly implement the shared policy across the affected scripts or clearly explain the narrower/duplicate boundary.
Validation run:
python -m pytest tests\test_gh_collection_caps.py -q-> 1 passed.python -m ruff check scripts\gh_collection_caps.py tests\test_gh_collection_caps.py-> passed.python -m ruff format --check scripts\gh_collection_caps.py tests\test_gh_collection_caps.py-> passed.git diff --check origin/main...HEAD-> clean.
Please wire the affected live scripts to the shared cap policy, align/document exceptions, and add focused saturation behavior coverage before merge.
|
@qingfeng312 — All CI checks green on Ready for review/merge when convenient. Wallet: |
1 similar comment
|
@qingfeng312 — All CI checks green on Ready for review/merge when convenient. Wallet: |
|
@taherdhanera — Safety-cap wiring + formatting feedback addressed on Wallet: |
|
@qingfeng312 — Safety-cap policy unification complete on Wallet: |
|
I rechecked this after the ping. The head is still 1f5c28e, 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 shared cap policy still needs to be wired into the affected live scripts before I can clear it. |
|
@qingfeng312 Follow-up on #1141 — added scripts/gh_collection_caps.py and tests/test_gh_collection_caps.py and normalized cap imports across maintenance scripts. Ready for re-review. |
yanyishuai
force-pushed
the
bounty-1141-gh-collection-caps
branch
from
1f5c28e to
f7bfa26
Compare
3 participants
Summary
Implements proposed work for #1141.
eview_bounty_candidates.py, check_live_bounty_closing_refs.py, and submission_quality_gate.py.
Test plan
Closes #1141
Summary by CodeRabbit
New Features
Bug Fixes