Refs #406: Use API bounty capacity in queue health#474
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:
📝 WalkthroughWalkthroughAdds MergeWork API enrichment for GitHub "bounty" issues: new loader with validation and safety cap, integration into load_live_queue() with metadata and fallback warnings, updates to analyze/formatters to surface bounty source and warnings, and tests for success and degraded API responses. ChangesAPI-driven bounty enrichment for queue health analysis
Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Reviewed PR #474 at current head I would hold this for one small observability gap before merge: Evidence on this head:
Suggested fix: keep the fallback, but include an Assessment: this is a good direction for MergeWork because queue-health should use actual award capacity, but without a surfaced fallback warning the tool can still produce overconfident payability output when the API layer is unavailable. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f2c0ce42-8a2c-4a43-81c1-9f453bfe2cac
📒 Files selected for processing (2)
scripts/pr_queue_health.pytests/test_pr_queue_health.py
|
Thanks, agreed. I pushed What changed:
Fresh validation:
|
|
Pushed Changes:
Fresh validation:
|
GHX5T-SOL
left a comment
GHX5T-SOL
left a comment
There was a problem hiding this comment.
Reviewed current head 1935aa2d526ed9c7589da0556d725bfa0a19ad5d.
No blocker found in this current-head pass. The earlier silent-fallback concern has been addressed: API failures now carry data_sources.bounties = github_issues plus api_bounty_warning, and text/markdown reports render the degraded source. The latest head also validates matching API rows and fails over at the 200-row safety cap instead of silently trusting a potentially truncated list.
Evidence checked:
- PR is open, non-draft, mergeable/CLEAN, with GitHub CI and CodeRabbit successful.
- Diff is scoped to
scripts/pr_queue_health.pyandtests/test_pr_queue_health.py. _load_api_bounties()now wraps malformed UTF-8/JSON/API failures, validatesstatusandawards_remaining, and records visible fallback metadata throughload_live_queue()/analyze_queue().- Live read-only queue health completed with
data_sources.bounties = github_issues+mergework_api. - Live API scan returned 76
ramimbo/mergeworkbounty rows and no duplicate(repo, issue_number)keys; the app model also has a uniqueness constraint for that pair.
Validation:
/home/kali/money/mergework/.venv/bin/python -m pytest tests/test_pr_queue_health.py -q-> 14 passed/home/kali/money/mergework/.venv/bin/python -m pytest -q-> 419 passed/home/kali/money/mergework/.venv/bin/python -m ruff check scripts/pr_queue_health.py tests/test_pr_queue_health.py-> passed/home/kali/money/mergework/.venv/bin/python -m ruff format --check scripts/pr_queue_health.py tests/test_pr_queue_health.py-> 2 files already formatted/home/kali/money/mergework/.venv/bin/python -m mypy app scripts/pr_queue_health.py-> success, no issues in 31 source filesgitleaks detect --no-banner --redact --source . --log-opts upstream/main..HEAD --exit-code 1-> no leaks found
|
Reviewed PR #474 at current head No blocker found in this current-head pass. The earlier silent-fallback risk is addressed: API bounty failures now carry Validation run on this head:
GitHub readback shows the PR is open, non-draft, |
eliasx45
left a comment
eliasx45
left a comment
There was a problem hiding this comment.
Reviewed PR #474 at head 1935aa2d526ed9c7589da0556d725bfa0a19ad5d for the queue-health API bounty-capacity overlay and fallback-reporting behavior.
The current implementation looks good in isolation: the focused tests cover API-backed bounty capacity, invalid API responses, visible fallback warning/source metadata, missing required API fields, and the 200-row safety-cap fallback. I do not see a behavior blocker in the submitted script/test slice.
Blocking issue:
- The branch is now unmergeable against current
origin/main. - Reproduced locally with
git merge --no-commit --no-ff origin/main:
Auto-merging scripts/pr_queue_health.py
CONFLICT (content): Merge conflict in scripts/pr_queue_health.py
Auto-merging tests/test_pr_queue_health.py
Automatic merge failed; fix conflicts and then commit the result.
Please rebase/merge current main and resolve the queue-health overlap before this can land.
Validation run locally on the PR head:
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 .\.venv\Scripts\python.exe -m pytest tests\test_pr_queue_health.py -q-> 14 passed.\.venv\Scripts\python.exe -m ruff check scripts\pr_queue_health.py tests\test_pr_queue_health.py-> all checks passed.\.venv\Scripts\python.exe -m ruff format --check scripts\pr_queue_health.py tests\test_pr_queue_health.py-> 2 files already formattedPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 .\.venv\Scripts\python.exe -m mypy app scripts\pr_queue_health.py-> successPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 .\.venv\Scripts\python.exe scripts\docs_smoke.py-> docs smoke okgit diff --check origin/main...HEAD-> clean
Requesting changes for the merge conflict, not for the queue-health API/fallback behavior itself.
|
Maintainer cleanup note: holding this until the branch is rebased or otherwise updated. The branch currently conflicts with |
…llback # Conflicts: # scripts/pr_queue_health.py
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 (1)
scripts/pr_queue_health.py (1)
345-357:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject out-of-range numeric API values before trusting the row.
app/serializers.pyclampsawards_remainingto>= 0, so a negative count here is malformed input, not authoritative state. Right now-1/"-1"passes validation and_is_open_bounty()turns it into “not payable”, which recreates the false-positive path this fallback boundary is meant to avoid. The same applies toissue_number <= 0, which can skew the summary counts.Proposed fix
issue_number = item.get("issue_number") if isinstance(issue_number, bool) or not isinstance(issue_number, int): raise RuntimeError(f"MergeWork API bounty row {index} has invalid issue_number") + if issue_number <= 0: + raise RuntimeError(f"MergeWork API bounty row {index} has invalid issue_number") status = item.get("status") if not isinstance(status, str) or not status.strip(): raise RuntimeError(f"MergeWork API bounty row {index} missing status") awards_remaining = item.get("awards_remaining") if awards_remaining is None: @@ try: normalized_awards_remaining = int(awards_remaining) except (TypeError, ValueError) as exc: raise RuntimeError( f"MergeWork API bounty row {index} missing awards_remaining" ) from exc + if normalized_awards_remaining < 0: + raise RuntimeError( + f"MergeWork API bounty row {index} has invalid awards_remaining" + )
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7ac39150-9480-4abb-8f54-9b57caa6fe97
📒 Files selected for processing (2)
scripts/pr_queue_health.pytests/test_pr_queue_health.py
|
Thanks for the cleanup note. I merged current Validation after the update:
GitHub now reports the PR as mergeable/clean on the updated head. |
eliasx45
left a comment
eliasx45
left a comment
There was a problem hiding this comment.
Reviewed current head 30f2b28cbf852e08fb75ea2036af792d31b009ed after the branch refresh.
Verdict: request changes.
The earlier merge-readiness blocker is fixed:
git diff --check origin/main...HEAD
# clean
git merge-tree --write-tree origin/main HEAD
# 6132f7c64961cbb5f8bdf5a7d27084d37efba80a
The focused suite also passes on this head:
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 python -m pytest tests/test_pr_queue_health.py -q
# 20 passed
python -m py_compile scripts/pr_queue_health.py
# passed
python -m ruff check scripts/pr_queue_health.py tests/test_pr_queue_health.py
# All checks passed
However, CodeRabbit's new API-row validation concern is reproducible and I agree it should block before merge. _load_api_bounties() currently accepts out-of-range API values as authoritative:
negative_awards {406: {'number': 406, 'title': 'b', 'state': 'open', 'awards_remaining': -1}}
zero_issue {0: {'number': 0, 'title': 'b', 'state': 'open', 'awards_remaining': 1}}
That matters because this PR's purpose is to avoid stale/incorrect payability conclusions by trusting API bounty capacity only after validation. A negative awards_remaining should be treated as malformed input and fall back with an api_bounty_warning, not converted into a non-payable authoritative row. Similarly, issue_number <= 0 should be rejected instead of entering the bounty map.
Requested fix: reject issue_number <= 0 and normalized_awards_remaining < 0 in _load_api_bounties(), and add regressions proving those malformed API rows trigger the warning/fallback path rather than being trusted.
|
Addressed the new API numeric-bound review in Changes:
Validation:
|
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8a3fdfa2-2b37-4227-9221-4baa204a4a51
📒 Files selected for processing (2)
scripts/pr_queue_health.pytests/test_pr_queue_health.py
eliasx45
left a comment
eliasx45
left a comment
There was a problem hiding this comment.
Reviewed current head 2a630fdf0589d5e8addf0904e0cdf6639f5dc755 after the API numeric-bound fix.
Verdict: request changes.
The behavioral fix itself looks correct: _load_api_bounties() now rejects issue_number <= 0 and negative awards_remaining instead of trusting those malformed API rows.
Focused validation:
git merge-tree --write-tree origin/main HEAD
# 6393c6607a68abc366a185303ef456b95c26a9cc
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ..\mergework\.venv\Scripts\python.exe -m pytest tests\test_pr_queue_health.py -q
# 22 passed
..\mergework\.venv\Scripts\python.exe -m py_compile scripts\pr_queue_health.py
# passed
..\mergework\.venv\Scripts\python.exe -m ruff check scripts\pr_queue_health.py tests\test_pr_queue_health.py
# All checks passed
The remaining blocker is formatting, which also matches the failing GitHub CI job:
..\mergework\.venv\Scripts\python.exe -m ruff format --check scripts\pr_queue_health.py tests\test_pr_queue_health.py
# Would reformat: scripts\pr_queue_health.py
# 1 file would be reformatted, 1 file already formatted
GitHub Actions failure readback for run 26620301091 shows the same issue:
ruff format --check .
Would reformat: scripts/pr_queue_health.py
1 file would be reformatted, 86 files already formatted
Requested fix: run Ruff format on scripts/pr_queue_health.py and push the formatting-only update. I do not see a remaining logic blocker after that.
|
Addressed the current-head Ruff formatting blocker in Validation:
|
eliasx45
left a comment
eliasx45
left a comment
There was a problem hiding this comment.
Re-reviewed current head 03a44f076c83eb1c180f2f9b4a50824b34b28cc3 after the Ruff formatting follow-up.
Verdict: approve.
The prior blocker is resolved: scripts/pr_queue_health.py is now Ruff-formatted, and the API numeric-bound behavior from the previous head remains covered.
Validation on this checkout:
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ..\mergework\.venv\Scripts\python.exe -m pytest tests\test_pr_queue_health.py -q
# 22 passed
..\mergework\.venv\Scripts\python.exe -m py_compile scripts\pr_queue_health.py
# passed
..\mergework\.venv\Scripts\python.exe -m ruff check scripts\pr_queue_health.py tests\test_pr_queue_health.py
# All checks passed
..\mergework\.venv\Scripts\python.exe -m ruff format --check scripts\pr_queue_health.py tests\test_pr_queue_health.py
# 2 files already formatted
git diff --check origin/main...HEAD
# clean
git merge-tree --write-tree origin/main HEAD
# 9a9a0e9977c6e24035419df37fb89c62c4cf0cb0
I do not see a remaining blocker in this queue-health API bounty-capacity slice.
|
Re-checked current head 03a44f0. The origin/main conflict blocker is cleared, and the later Ruff formatting blocker is cleared. Waiting for a second useful current-head review before merge. |
6 participants
Summary
pr_queue_healthlive mode with public MergeWork API bounty metadata for the target repostatusandawards_remainingto avoid treating exhausted open GitHub bounty issues as payableRefs #406.
Evidence
This addresses a real queue-health edge case: GitHub issue state alone can show a bounty issue as open while the public MergeWork API reports no remaining award capacity. Without the API overlay, live queue reports can incorrectly mark PRs against exhausted bounties as still payable. The invalid UTF-8 regression keeps the fallback boundary intact for malformed API responses.
Validation
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ~/.local/bin/uv run --extra dev python -m pytest tests/test_pr_queue_health.py -q->14 passedPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ~/.local/bin/uv run --extra dev python -m pytest -q->419 passed~/.local/bin/uv run --extra dev ruff check .-> passed~/.local/bin/uv run --extra dev ruff format --check .->79 files already formatted~/.local/bin/uv run --extra dev mypy app scripts/pr_queue_health.py-> successgit diff --check-> cleanNote: pytest plugin autoload is disabled because this workstation has an unrelated ROS Humble pytest plugin on the global path.
Summary by CodeRabbit
New Features
Improvements
Tests