Use API award capacity in queue health#472
Conversation
📝 WalkthroughWalkthroughThis PR enhances bounty queue health monitoring by integrating API-provided metadata. The script fetches bounty state and award capacity from a MergeWork API endpoint, merging that data into the live queue analysis to override locally-derived bounty states when available. ChangesAPI bounty metadata integration
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 |
er1c-cartman
left a comment
er1c-cartman
left a comment
There was a problem hiding this comment.
Reviewed current head a8319aad13366ff4fbffb596be60f329041463a6 for the #447 review bounty.
I inspected scripts/pr_queue_health.py, tests/test_pr_queue_health.py, and the existing queue analysis path. The change is scoped: live mode still gathers PRs/issues through gh, then overlays only the bounty state and award capacity from the public MergeWork API when that API is available. If the API is unavailable or malformed, the existing GitHub-only heuristic is preserved.
Checks I walked through:
- API
statusvalues are lowercase (open) while GitHub issue state is uppercase (OPEN);_is_open_bounty()normalizes both. - API
awards_remaining: 0flows throughanalyze_queue()and marks a referenced bounty as not payable, which is the behavior this patch needs. - API failures intentionally fall back to the previous GitHub issue-state behavior.
- Live
_load_api_bounties("ramimbo/mergework")returned current rows for #406, #447, and #459, including current award capacity.
Validation:
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ~/.local/bin/uv run --extra dev python -m pytest tests/test_pr_queue_health.py -q->10 passedPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ~/.local/bin/uv run --extra dev ruff check scripts/pr_queue_health.py tests/test_pr_queue_health.py-> passedPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ~/.local/bin/uv run --extra dev ruff format --check scripts/pr_queue_health.py tests/test_pr_queue_health.py->2 files already formattedgit diff --check origin/main...HEAD-> clean- live API readback: #406 open/15 awards, #447 open/8 awards, #459 open/10 awards, 76 repo rows
Note: I disabled external pytest plugin autoload because this workstation has a ROS Humble pytest plugin on the global path; the project test itself passed after isolating third-party pytest plugins.
No blocker found in this scoped review.
![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: f060d644-00ae-4bf7-9e31-256f5f86a337
📒 Files selected for processing (2)
scripts/pr_queue_health.pytests/test_pr_queue_health.py
| payload = json.loads(response.read().decode("utf-8")) | ||
| except (OSError, URLError, json.JSONDecodeError) as exc: | ||
| raise RuntimeError(f"MergeWork API bounty data unavailable: {exc}") from exc |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Confirm current exception list in the reviewed file.
rg -n "except \\(OSError, URLError, .*json\\.JSONDecodeError\\)" scripts/pr_queue_health.py
# 2) Confirm Python exception hierarchy relevant to decoding.
python - <<'PY'
print("UnicodeDecodeError subclass of ValueError:", issubclass(UnicodeDecodeError, ValueError))
print("UnicodeDecodeError subclass of OSError:", issubclass(UnicodeDecodeError, OSError))
PYRepository: ramimbo/mergework
Length of output: 220
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect decode + exception handling in the reviewed function
sed -n '270,380p' scripts/pr_queue_health.py | nl -ba | sed -n '1,130p'
# 2) Focus search for load_live_queue fallback exception handling
rg -n "def load_live_queue|load_live_queue\(|except|_load_api_bounties" scripts/pr_queue_health.py
# 3) Repo coding guidelines checks
ruff format --check .
ruff check .Repository: ramimbo/mergework
Length of output: 104
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Print relevant section with line numbers (use cat -n since nl may not exist)
cat -n scripts/pr_queue_health.py | sed -n '250,420p'
# 2) Find definitions and exception handling in the relevant functions
rg -n "def (_load_api_bounties|load_live_queue)\b|load_live_queue\(|_load_api_bounties\(|except " scripts/pr_queue_health.py
# 3) Run required Python quality/format checks (per guidelines)
ruff format --check .
ruff check .Repository: ramimbo/mergework
Length of output: 7302
Handle UTF-8 decode failures in scripts/pr_queue_health.py so live fallback still works
_load_api_bounties() decodes at line 291, but the handler at lines 292-293 only catches (OSError, URLError, json.JSONDecodeError). If decode("utf-8") raises UnicodeDecodeError, it bypasses the RuntimeError wrapper and load_live_queue()’s except RuntimeError fallback.
Proposed fix
- except (OSError, URLError, json.JSONDecodeError) as exc:
+ except (OSError, URLError, UnicodeDecodeError, json.JSONDecodeError) as exc:
raise RuntimeError(f"MergeWork API bounty data unavailable: {exc}") from exc| def test_pr_queue_health_live_mode_uses_api_award_capacity(monkeypatch) -> None: | ||
| def fake_run(args, **kwargs): | ||
| if args[:3] == ["gh", "pr", "list"]: | ||
| stdout = json.dumps( | ||
| [ | ||
| { | ||
| "number": 71, | ||
| "title": "Late fix for exhausted bounty", | ||
| "body": "Refs #406", | ||
| "labels": [], | ||
| "mergeStateStatus": "clean", | ||
| "url": "https://github.com/ramimbo/mergework/pull/71", | ||
| } | ||
| ] | ||
| ) | ||
| elif args[:3] == ["gh", "issue", "list"]: | ||
| stdout = json.dumps( | ||
| [ | ||
| { | ||
| "number": 406, | ||
| "title": "MRWK bounty: useful bug reports", | ||
| "state": "OPEN", | ||
| } | ||
| ] | ||
| ) | ||
| else: | ||
| raise AssertionError(args) | ||
| return subprocess.CompletedProcess(args=args, returncode=0, stdout=stdout, stderr="") | ||
|
|
||
| class FakeResponse: | ||
| def __enter__(self): | ||
| return self | ||
|
|
||
| def __exit__(self, *args): | ||
| return None | ||
|
|
||
| def read(self): | ||
| return json.dumps( | ||
| [ | ||
| { | ||
| "repo": "ramimbo/mergework", | ||
| "issue_number": 406, | ||
| "title": "MRWK bounty: useful bug reports", | ||
| "status": "open", | ||
| "awards_remaining": 0, | ||
| } | ||
| ] | ||
| ).encode() | ||
|
|
||
| def fake_urlopen(url, timeout): | ||
| assert url == "https://api.mrwk.ltclab.site/api/v1/bounties?limit=200" | ||
| assert timeout == pr_queue_health.GH_TIMEOUT_SECONDS | ||
| return FakeResponse() | ||
|
|
||
| monkeypatch.setattr(pr_queue_health.subprocess, "run", fake_run) | ||
| monkeypatch.setattr(pr_queue_health, "urlopen", fake_urlopen, raising=False) | ||
|
|
||
| report = analyze_queue(pr_queue_health.load_live_queue("ramimbo/mergework")) | ||
|
|
||
| assert report["summary"]["closed_bounty_references"] == 1 | ||
| assert report["closed_bounty_references"][0]["pull_request"] == 71 | ||
| assert report["closed_bounty_references"][0]["detail"] == ( | ||
| "Referenced bounty #406 is not payable" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add a fallback-path regression test for API failure.
This test covers API-success override, but not the new failure path (urlopen error) that should preserve GitHub-derived bounty capacity.
Suggested additional test
+def test_pr_queue_health_live_mode_falls_back_when_api_unavailable(monkeypatch) -> None:
+ def fake_run(args, **kwargs):
+ if args[:3] == ["gh", "pr", "list"]:
+ stdout = json.dumps(
+ [
+ {
+ "number": 72,
+ "title": "Fallback path still payable",
+ "body": "Refs `#406`",
+ "labels": [],
+ "mergeStateStatus": "clean",
+ }
+ ]
+ )
+ elif args[:3] == ["gh", "issue", "list"]:
+ stdout = json.dumps(
+ [{"number": 406, "title": "MRWK bounty: useful bug reports", "state": "OPEN"}]
+ )
+ else:
+ raise AssertionError(args)
+ return subprocess.CompletedProcess(args=args, returncode=0, stdout=stdout, stderr="")
+
+ def fake_urlopen(url, timeout):
+ raise pr_queue_health.URLError("temporary outage")
+
+ monkeypatch.setattr(pr_queue_health.subprocess, "run", fake_run)
+ monkeypatch.setattr(pr_queue_health, "urlopen", fake_urlopen, raising=False)
+
+ report = analyze_queue(pr_queue_health.load_live_queue("ramimbo/mergework"))
+ assert report["summary"]["closed_bounty_references"] == 0As per coding guidelines, "tests/**/*.py: Do not request docstrings. Focus on whether tests prove the changed behavior and include negative, replay, boundary, or regression cases where relevant."
📝 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_pr_queue_health_live_mode_uses_api_award_capacity(monkeypatch) -> None: | |
| def fake_run(args, **kwargs): | |
| if args[:3] == ["gh", "pr", "list"]: | |
| stdout = json.dumps( | |
| [ | |
| { | |
| "number": 71, | |
| "title": "Late fix for exhausted bounty", | |
| "body": "Refs #406", | |
| "labels": [], | |
| "mergeStateStatus": "clean", | |
| "url": "https://github.com/ramimbo/mergework/pull/71", | |
| } | |
| ] | |
| ) | |
| elif args[:3] == ["gh", "issue", "list"]: | |
| stdout = json.dumps( | |
| [ | |
| { | |
| "number": 406, | |
| "title": "MRWK bounty: useful bug reports", | |
| "state": "OPEN", | |
| } | |
| ] | |
| ) | |
| else: | |
| raise AssertionError(args) | |
| return subprocess.CompletedProcess(args=args, returncode=0, stdout=stdout, stderr="") | |
| class FakeResponse: | |
| def __enter__(self): | |
| return self | |
| def __exit__(self, *args): | |
| return None | |
| def read(self): | |
| return json.dumps( | |
| [ | |
| { | |
| "repo": "ramimbo/mergework", | |
| "issue_number": 406, | |
| "title": "MRWK bounty: useful bug reports", | |
| "status": "open", | |
| "awards_remaining": 0, | |
| } | |
| ] | |
| ).encode() | |
| def fake_urlopen(url, timeout): | |
| assert url == "https://api.mrwk.ltclab.site/api/v1/bounties?limit=200" | |
| assert timeout == pr_queue_health.GH_TIMEOUT_SECONDS | |
| return FakeResponse() | |
| monkeypatch.setattr(pr_queue_health.subprocess, "run", fake_run) | |
| monkeypatch.setattr(pr_queue_health, "urlopen", fake_urlopen, raising=False) | |
| report = analyze_queue(pr_queue_health.load_live_queue("ramimbo/mergework")) | |
| assert report["summary"]["closed_bounty_references"] == 1 | |
| assert report["closed_bounty_references"][0]["pull_request"] == 71 | |
| assert report["closed_bounty_references"][0]["detail"] == ( | |
| "Referenced bounty #406 is not payable" | |
| ) | |
| def test_pr_queue_health_live_mode_uses_api_award_capacity(monkeypatch) -> None: | |
| def fake_run(args, **kwargs): | |
| if args[:3] == ["gh", "pr", "list"]: | |
| stdout = json.dumps( | |
| [ | |
| { | |
| "number": 71, | |
| "title": "Late fix for exhausted bounty", | |
| "body": "Refs `#406`", | |
| "labels": [], | |
| "mergeStateStatus": "clean", | |
| "url": "https://github.com/ramimbo/mergework/pull/71", | |
| } | |
| ] | |
| ) | |
| elif args[:3] == ["gh", "issue", "list"]: | |
| stdout = json.dumps( | |
| [ | |
| { | |
| "number": 406, | |
| "title": "MRWK bounty: useful bug reports", | |
| "state": "OPEN", | |
| } | |
| ] | |
| ) | |
| else: | |
| raise AssertionError(args) | |
| return subprocess.CompletedProcess(args=args, returncode=0, stdout=stdout, stderr="") | |
| class FakeResponse: | |
| def __enter__(self): | |
| return self | |
| def __exit__(self, *args): | |
| return None | |
| def read(self): | |
| return json.dumps( | |
| [ | |
| { | |
| "repo": "ramimbo/mergework", | |
| "issue_number": 406, | |
| "title": "MRWK bounty: useful bug reports", | |
| "status": "open", | |
| "awards_remaining": 0, | |
| } | |
| ] | |
| ).encode() | |
| def fake_urlopen(url, timeout): | |
| assert url == "https://api.mrwk.ltclab.site/api/v1/bounties?limit=200" | |
| assert timeout == pr_queue_health.GH_TIMEOUT_SECONDS | |
| return FakeResponse() | |
| monkeypatch.setattr(pr_queue_health.subprocess, "run", fake_run) | |
| monkeypatch.setattr(pr_queue_health, "urlopen", fake_urlopen, raising=False) | |
| report = analyze_queue(pr_queue_health.load_live_queue("ramimbo/mergework")) | |
| assert report["summary"]["closed_bounty_references"] == 1 | |
| assert report["closed_bounty_references"][0]["pull_request"] == 71 | |
| assert report["closed_bounty_references"][0]["detail"] == ( | |
| "Referenced bounty `#406` is not payable" | |
| ) | |
| def test_pr_queue_health_live_mode_falls_back_when_api_unavailable(monkeypatch) -> None: | |
| def fake_run(args, **kwargs): | |
| if args[:3] == ["gh", "pr", "list"]: | |
| stdout = json.dumps( | |
| [ | |
| { | |
| "number": 72, | |
| "title": "Fallback path still payable", | |
| "body": "Refs `#406`", | |
| "labels": [], | |
| "mergeStateStatus": "clean", | |
| } | |
| ] | |
| ) | |
| elif args[:3] == ["gh", "issue", "list"]: | |
| stdout = json.dumps( | |
| [{"number": 406, "title": "MRWK bounty: useful bug reports", "state": "OPEN"}] | |
| ) | |
| else: | |
| raise AssertionError(args) | |
| return subprocess.CompletedProcess(args=args, returncode=0, stdout=stdout, stderr="") | |
| def fake_urlopen(url, timeout): | |
| raise pr_queue_health.URLError("temporary outage") | |
| monkeypatch.setattr(pr_queue_health.subprocess, "run", fake_run) | |
| monkeypatch.setattr(pr_queue_health, "urlopen", fake_urlopen, raising=False) | |
| report = analyze_queue(pr_queue_health.load_live_queue("ramimbo/mergework")) | |
| assert report["summary"]["closed_bounty_references"] == 0 |
adliebe
left a comment
adliebe
left a comment
There was a problem hiding this comment.
Reviewed PR #472 at current head a8319aad13366ff4fbffb596be60f329041463a6.
I found one fallback-path blocker before this should merge. The PR intends to preserve the GitHub-only fallback when the MergeWork API is unavailable or malformed, but _load_api_bounties() currently does not wrap UnicodeDecodeError from response.read().decode("utf-8"). That means a bad/non-UTF-8 API response bypasses the RuntimeError contract and load_live_queue() does not reach its except RuntimeError: api_bounties = {} fallback.
Concrete repro:
class BadUtf8Response:
def __enter__(self): return self
def __exit__(self, *args): return None
def read(self): return b"\xff\xfe"
pr_queue_health.urlopen = lambda url, timeout: BadUtf8Response()
pr_queue_health._load_api_bounties("ramimbo/mergework")
Observed result:
UnicodeDecodeError
'utf-8' codec can't decode byte 0xff in position 0: invalid start byte
The same bad response through load_live_queue("ramimbo/mergework") also raises UnicodeDecodeError, so the fallback is not preserved for that malformed-response class. A simulated URLError("temporary outage") does fall back correctly and produced closed_bounty_references == 0, open_bounties == 1 in my local harness.
Recommended fix: catch UnicodeDecodeError alongside OSError, URLError, and json.JSONDecodeError, then add a regression test for API fallback behavior. Keep the test fixture using Refs #406; a backticked Refs \#406`form does not match the currentBOUNTY_REF_RE`.
Validation I ran on the current head:
.venv\Scripts\python.exe -m pytest tests\test_pr_queue_health.py -q-> 10 passed.venv\Scripts\ruff.exe check scripts\pr_queue_health.py tests\test_pr_queue_health.py-> All checks passed.venv\Scripts\ruff.exe format --check scripts\pr_queue_health.py tests\test_pr_queue_health.py-> 2 files already formatted.venv\Scripts\python.exe -m mypy app scripts\pr_queue_health.py-> Success, no issues found in 31 source files.venv\Scripts\python.exe -m pytest -q-> 415 passedgit diff --check origin/main...HEAD-> clean
The successful API overlay path looks good; the failure/fallback boundary just needs the decode case covered.
GHX5T-SOL
left a comment
GHX5T-SOL
left a comment
There was a problem hiding this comment.
Reviewed current head a8319aad13366ff4fbffb596be60f329041463a6.
I found one additional blocker separate from the existing UTF-8/fallback comments. _load_api_bounties() collapses API rows into a dict by issue_number, but it overwrites an existing entry every time it sees the same issue. The live endpoint currently returns newest rows first (limit=10 starts with ids 76, 75, 74...), so if the API ever contains two bounty rows for the same repo issue, an older/stale row later in the response wins over the newer/current row. That can make load_live_queue() report an open/payable bounty as closed or exhausted.
Local repro against the submitted code:
p.urlopen = lambda url, timeout: response_with([
{"repo":"ramimbo/mergework","issue_number":406,"title":"new","status":"open","awards_remaining":5},
{"repo":"ramimbo/mergework","issue_number":406,"title":"old","status":"paid","awards_remaining":0},
])
print(p._load_api_bounties("ramimbo/mergework"))
Observed output:
{406: {'number': 406, 'title': 'old', 'state': 'paid', 'awards_remaining': 0}}
That is the stale row, even though the current/newer row appeared first. I would either keep the first row for a repo/issue pair after filtering, or explicitly compare a stable API field such as id/created_at if this endpoint does not guarantee ordering. A regression test with duplicate API rows would catch the current overwrite behavior.
Other validation I ran on this head:
/home/kali/money/mergework/.venv/bin/python -m pytest tests/test_pr_queue_health.py -q-> 10 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 formattedgit diff --check upstream/main...HEAD-> clean- diff gitleaks scan -> no leaks found
- live
scripts/pr_queue_health.py --repo ramimbo/mergework --format jsoncompleted; current API scan returned 76 rows and no duplicate(repo, issue_number)pairs at review time
GHX5T-SOL
left a comment
GHX5T-SOL
left a comment
There was a problem hiding this comment.
Reviewed current head a8319aad13366ff4fbffb596be60f329041463a6.
I am requesting changes because the new API fallback is incomplete. The PR says live queue health preserves the GitHub-only fallback when the MergeWork API is unavailable, and load_live_queue() only falls back when _load_api_bounties() raises RuntimeError. However _load_api_bounties() decodes the response before json.loads, and UnicodeDecodeError is not caught or wrapped. A malformed/non-UTF-8 API response therefore aborts live queue collection instead of falling back to GitHub-derived bounty state.
Local reproduction against this head:
UnicodeDecodeError 'utf-8' codec can't decode byte 0xff in position 0: invalid start byte
The repro monkeypatched urlopen() to return b'\xff' and monkeypatched the gh calls to return one open PR referencing open bounty #406. Expected behavior for an unavailable/unusable API response is the same GitHub-only fallback used for URLError; actual behavior is an uncaught decode exception.
Suggested fix: include UnicodeDecodeError in the _load_api_bounties() wrapper and add a regression test for the fallback path. A focused test should prove that when the API raises URLError or returns undecodable bytes, load_live_queue() still returns the GitHub-derived open bounty with no closed-bounty reference.
Validation I ran:
uv run --extra dev python -m pytest tests/test_pr_queue_health.py -q
# 10 passed
uv run --extra dev python -m pytest -q
# 415 passed
uv run --extra dev python scripts/docs_smoke.py
# docs smoke ok
uv run --extra dev ruff check scripts/pr_queue_health.py tests/test_pr_queue_health.py
# All checks passed!
uv run --extra dev ruff format --check scripts/pr_queue_health.py tests/test_pr_queue_health.py
# 2 files already formatted
uv run --extra dev python -m mypy app scripts/pr_queue_health.py
# Success: no issues found in 31 source files
git diff --check origin/main...HEAD
# clean
gitleaks detect --no-banner --redact --source . --log-opts origin/main..HEAD --exit-code 1
# no leaks found
Live readback using this branch also confirmed the success path is wired to the public API: #406 reads as open with 15 awards remaining, #447 as open with 8, and #459 as open with 10. The blocker is specifically the fallback/error path.
4 participants
Refs #406
Summary
scripts/pr_queue_health.py --repo ...read MergeWork bounty capacity from the public bounty API.awards_remainingis0.Why
The runbook says queue health reports closed or exhausted bounty references before maintainer batches. Before this change, live mode only looked at GitHub issue state and treated every open bounty issue as having one payable slot, so an open but exhausted bounty could be missed.
Evidence
GET https://api.mrwk.ltclab.site/api/v1/bounties/66returned issue#406, statusopen,awards_remaining: 15,available_mrwk: "750".GET /api/v1/bounties/66/attempts?include_expired=falsereturned no warnings and no active attempts./claim #Nreference parsing.Validation
uv run --extra dev python -m pytest tests/test_pr_queue_health.py -q-> 10 passeduv run --extra dev ruff check scripts/pr_queue_health.py tests/test_pr_queue_health.py-> passeduv run --extra dev ruff format --check scripts/pr_queue_health.py tests/test_pr_queue_health.py-> passeduv run --extra dev python -m mypy app scripts/pr_queue_health.py-> successgit diff --check-> cleanSummary by CodeRabbit
Release Notes
New Features
Tests