Add shared public JSON fetch helper for maintenance scripts (#1144)#1174
Add shared public JSON fetch helper for maintenance scripts (#1144)#1174yanyishuai wants to merge 1 commit into
Conversation
|
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 ChangesPublic JSON fetch consolidation
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 |
qingfeng312
left a comment
qingfeng312
left a comment
There was a problem hiding this comment.
Finding:
- [P1] Fix the ruff failures introduced by the new public JSON helper before merge. The current head
31692e94afc796a6256c17d084ab5c038acf3b05passes pytest (911 passed, 1 warning), but CI fails atruff check .with six issues:scripts/claim_inventory.pystill imports unusedurllib.request, its local script imports are unsorted,tests/test_public_json_fetch.pyhas a quoted_FakeResponsereturn annotation, and three nestedwith patch(...): with pytest.raises(...)blocks triggerSIM117. Please remove the stale import, organize the imports, and apply the test-style cleanups so the lint gate passes.
Evidence checked:
- PR #1174 is open, non-draft, and authored by another account.
- Inspected
scripts/claim_inventory.py,scripts/public_json_fetch.py, andtests/test_public_json_fetch.py. - Checked CI run
28329923637: pytest passed, formatting passed, thenruff check .failed with the listed errors.
Scope boundary: review evidence only. No wallet signing, treasury mutation, payout execution, exchange, bridge, private data, credentials, or secrets involved.
yanyishuai
force-pushed
the
fix/issue-1144-shared-json-fetch
branch
from
31692e9 to
4038298
Compare
|
@qingfeng312 Ruff issues fixed.
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 40382987dbf7bebd1f75ce85981c3d6f88297799.
The earlier Ruff lint blocker has been resolved. I rechecked scripts/public_json_fetch.py, the claim_inventory migration, and tests/test_public_json_fetch.py.
The new helper centralizes public JSON fetching without changing the existing _get_json caller contract, preserves the timeout wiring, and covers HTTP, timeout, invalid JSON, custom timeout, and header behavior in focused tests.
Validation checked: GitHub CI Quality, readiness, docs, and image checks passed on run 28349126882; CodeRabbit status is success on this head. I did not find a remaining blocker for the scoped #1144 change.
yanyishuai
force-pushed
the
fix/issue-1144-shared-json-fetch
branch
from
4038298 to
601f77c
Compare
qingfeng312
left a comment
qingfeng312
left a comment
There was a problem hiding this comment.
Reviewed current head 601f77cb0b1f2a8fc58be0412c94fcd3722ac34c.
Requesting changes because the current branch is no longer merge-ready. The hosted quality gate run 28493046454 passes pytest, then fails at ruff format --check .:
Would reformat: tests/test_public_json_fetch.py
The earlier lint blocker was fixed on a previous head, but this updated head reintroduces a required formatting failure. Please run Ruff formatting on the test file and rerun the quality workflow.
yanyishuai
force-pushed
the
fix/issue-1144-shared-json-fetch
branch
2 times, most recently
from
aefb6c4 to
a11580c
Compare
|
@qingfeng312 CI is green on the latest head ( |
1 similar comment
|
@qingfeng312 CI is green on the latest head ( |
|
Bounty #1009 current-head review for PR #1174. Reviewed head: Finding:
Evidence checked:
Scope boundary: review evidence only. No wallet signing, treasury mutation, payout execution, bridge/off-ramp behavior, private data, credentials, or secrets involved. |
qingfeng312
left a comment
qingfeng312
left a comment
There was a problem hiding this comment.
Reviewed current head a11580c2b77604ad7815343de887a69d667029d5.
Requesting changes. The hosted quality/readiness check is green, but the current patch still is not merge-ready because the newly added public JSON helper files are committed with CRLF line endings.
Evidence: the current patch carries carriage-return bytes on every added line in scripts/public_json_fetch.py and tests/test_public_json_fetch.py. That makes Git's whitespace checks report trailing whitespace across those files even though the Python tests and formatter-facing checks pass.
Please normalize the new files to LF line endings and keep the functional changes otherwise intact.
|
@qingfeng312 CI is green on the latest head ( |
|
@qingfeng312 Thanks for the review — pushed LF-normalized line endings for Head should now pass Wallet: |
yanyishuai
force-pushed
the
fix/issue-1144-shared-json-fetch
branch
from
a11580c to
b9293e7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/claim_inventory.py`:
- Around line 18-25: The three safety-cap constants in claim_inventory.py are
duplicated with the same literal value, so either define and use a single shared
constant for the cap or add a short comment near GH_PUBLIC_API_SAFETY_CAP,
GH_ISSUE_SAFETY_CAP, and GH_PR_SAFETY_CAP explaining that they are intentionally
kept separate for different surfaces; update the references in
claim_inventory.py accordingly.
In `@scripts/public_json_fetch.py`:
- Around line 30-31: The unbounded response body read in the public JSON fetch
flow can exhaust memory if the endpoint returns a huge payload. Update the
urllib.request.urlopen handling in the public JSON fetch script to enforce a
maximum payload size by reading with a cap and checking for overflow before
decoding. Keep the fix localized to the response.read logic in the fetch helper
so the script rejects oversized responses defensively.
- Around line 13-20: Add a docstring to load_public_json that states it is
read-only (it only fetches public JSON and never mutates remote state) and
explains its exception behavior. Make clear when PublicJsonError is raised, what
underlying failures it wraps, and which errors are propagated unchanged, so
callers can understand the contract from the function itself.
- Around line 29-39: Wrap the UTF-8 decode failure in PublicJsonError as well:
in public_json_fetch’s urlopen/read/decode flow, catch UnicodeDecodeError
alongside the existing HTTPError/URLError/TimeoutError handling and re-raise it
as a PublicJsonError with the same “{label} unavailable” style message, so all
fetch/parse failures are consistently surfaced from the helper.
In `@tests/test_public_json_fetch.py`:
- Around line 25-33: The two tests are duplicating the same FakeResponse helper,
so extract that shared response stub into a single module-level helper or
fixture and reuse it from both test_load_public_json_sets_default_headers and
test_load_public_json_honors_custom_timeout. Keep the behavior of FakeResponse
the same (the bytes-backed response object used by fake_urlopen), and update
both test functions to reference the shared helper instead of redefining it.
- Around line 22-40: Add coverage for the non-UTF-8 decode path in
load_public_json by introducing a test that uses a fake response body with
invalid UTF-8 bytes (for example, via the existing FakeResponse in
test_load_public_json_sets_default_headers) and asserts the call raises
PublicJsonError instead of leaking UnicodeDecodeError. Keep the test alongside
the current urllib.request.urlopen patching so it exercises the same
public_json_fetch behavior and verifies the new error wrapping once implemented.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9518f059-1615-45e4-b2f3-6d04f7b2f3e1
📒 Files selected for processing (3)
scripts/claim_inventory.pyscripts/public_json_fetch.pytests/test_public_json_fetch.py
| def load_public_json( | ||
| url: str, | ||
| *, | ||
| description: str | None = None, | ||
| timeout: float = 30, | ||
| user_agent: str = "mergework-maintenance-script", | ||
| accept: str = "application/json", | ||
| ) -> Any: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add a docstring documenting the read-only contract and exception semantics.
The PR objective explicitly calls for documenting load_public_json's read-only contract and exception semantics, but the function has no docstring (only the exception class does). Callers can't tell from the signature alone that it never mutates remote state, what PublicJsonError wraps, or when it's raised vs. propagated.
📝 Proposed docstring
def load_public_json(
url: str,
*,
description: str | None = None,
timeout: float = 30,
user_agent: str = "mergework-maintenance-script",
accept: str = "application/json",
) -> Any:
+ """Fetch and decode a public JSON endpoint via a read-only GET request.
+
+ Raises:
+ PublicJsonError: If the request fails (network/timeout/HTTP error)
+ or the response body is not valid JSON.
+ """
label = description or "public JSON"📝 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_public_json( | |
| url: str, | |
| *, | |
| description: str | None = None, | |
| timeout: float = 30, | |
| user_agent: str = "mergework-maintenance-script", | |
| accept: str = "application/json", | |
| ) -> Any: | |
| def load_public_json( | |
| url: str, | |
| *, | |
| description: str | None = None, | |
| timeout: float = 30, | |
| user_agent: str = "mergework-maintenance-script", | |
| accept: str = "application/json", | |
| ) -> Any: | |
| """Fetch and decode a public JSON endpoint via a read-only GET request. | |
| Raises: | |
| PublicJsonError: If the request fails (network/timeout/HTTP error) | |
| or the response body is not valid JSON. | |
| """ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/public_json_fetch.py` around lines 13 - 20, Add a docstring to
load_public_json that states it is read-only (it only fetches public JSON and
never mutates remote state) and explains its exception behavior. Make clear when
PublicJsonError is raised, what underlying failures it wraps, and which errors
are propagated unchanged, so callers can understand the contract from the
function itself.
| try: | ||
| with urllib.request.urlopen(request, timeout=timeout) as response: | ||
| payload = response.read().decode("utf-8") | ||
| except urllib.error.HTTPError as exc: | ||
| raise PublicJsonError(f"{label} unavailable: HTTP {exc.code}") from exc | ||
| except (urllib.error.URLError, TimeoutError) as exc: | ||
| raise PublicJsonError(f"{label} unavailable: {exc}") from exc | ||
| try: | ||
| return json.loads(payload) | ||
| except json.JSONDecodeError as exc: | ||
| raise PublicJsonError(f"{label} unavailable: invalid JSON") from exc |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Decode failures aren't wrapped in PublicJsonError.
response.read().decode("utf-8") at Line 31 sits inside the first try, but the except clauses only catch HTTPError/URLError/TimeoutError (Lines 32-35). A non-UTF-8 response body raises an uncaught UnicodeDecodeError, breaking the helper's uniform-error-wrapping contract that callers rely on.
🐛 Proposed fix
try:
with urllib.request.urlopen(request, timeout=timeout) as response:
- payload = response.read().decode("utf-8")
+ raw = response.read()
except urllib.error.HTTPError as exc:
raise PublicJsonError(f"{label} unavailable: HTTP {exc.code}") from exc
except (urllib.error.URLError, TimeoutError) as exc:
raise PublicJsonError(f"{label} unavailable: {exc}") from exc
+ try:
+ payload = raw.decode("utf-8")
+ except UnicodeDecodeError as exc:
+ raise PublicJsonError(f"{label} unavailable: invalid encoding") from exc
try:
return json.loads(payload)📝 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.
| try: | |
| with urllib.request.urlopen(request, timeout=timeout) as response: | |
| payload = response.read().decode("utf-8") | |
| except urllib.error.HTTPError as exc: | |
| raise PublicJsonError(f"{label} unavailable: HTTP {exc.code}") from exc | |
| except (urllib.error.URLError, TimeoutError) as exc: | |
| raise PublicJsonError(f"{label} unavailable: {exc}") from exc | |
| try: | |
| return json.loads(payload) | |
| except json.JSONDecodeError as exc: | |
| raise PublicJsonError(f"{label} unavailable: invalid JSON") from exc | |
| try: | |
| with urllib.request.urlopen(request, timeout=timeout) as response: | |
| raw = response.read() | |
| except urllib.error.HTTPError as exc: | |
| raise PublicJsonError(f"{label} unavailable: HTTP {exc.code}") from exc | |
| except (urllib.error.URLError, TimeoutError) as exc: | |
| raise PublicJsonError(f"{label} unavailable: {exc}") from exc | |
| try: | |
| payload = raw.decode("utf-8") | |
| except UnicodeDecodeError as exc: | |
| raise PublicJsonError(f"{label} unavailable: invalid encoding") from exc | |
| try: | |
| return json.loads(payload) | |
| except json.JSONDecodeError as exc: | |
| raise PublicJsonError(f"{label} unavailable: invalid JSON") from exc |
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 29-29: Request-controlled URL passed to urlopen; validate against an allowlist to prevent SSRF.
Context: urllib.request.urlopen(request, timeout=timeout)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(urlopen-unsanitized-data)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/public_json_fetch.py` around lines 29 - 39, Wrap the UTF-8 decode
failure in PublicJsonError as well: in public_json_fetch’s urlopen/read/decode
flow, catch UnicodeDecodeError alongside the existing
HTTPError/URLError/TimeoutError handling and re-raise it as a PublicJsonError
with the same “{label} unavailable” style message, so all fetch/parse failures
are consistently surfaced from the helper.
| with urllib.request.urlopen(request, timeout=timeout) as response: | ||
| payload = response.read().decode("utf-8") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoff
Unbounded response.read().
No size cap is applied when reading the response body, so a misbehaving or compromised endpoint could return an arbitrarily large payload and exhaust memory in these maintenance scripts. Given the "public JSON" scope, consider capping read size (e.g., response.read(MAX_BYTES + 1) with a length check) as a defensive measure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/public_json_fetch.py` around lines 30 - 31, The unbounded response
body read in the public JSON fetch flow can exhaust memory if the endpoint
returns a huge payload. Update the urllib.request.urlopen handling in the public
JSON fetch script to enforce a maximum payload size by reading with a cap and
checking for overflow before decoding. Keep the fix localized to the
response.read logic in the fetch helper so the script rejects oversized
responses defensively.
| def test_load_public_json_sets_default_headers() -> None: | ||
| captured: dict[str, str] = {} | ||
|
|
||
| class FakeResponse(io.BytesIO): | ||
| def __init__(self) -> None: | ||
| super().__init__(b"[]") | ||
|
|
||
| def fake_urlopen(request, timeout=30): # noqa: ANN001 | ||
| captured["accept"] = request.get_header("Accept") | ||
| captured["user_agent"] = request.get_header("User-agent") | ||
| captured["timeout"] = str(timeout) | ||
| return FakeResponse() | ||
|
|
||
| with patch("urllib.request.urlopen", side_effect=fake_urlopen): | ||
| load_public_json("https://example.test/api/v1/bounties") | ||
|
|
||
| assert captured["accept"] == "application/json" | ||
| assert captured["user_agent"] == "mergework-maintenance-script" | ||
| assert captured["timeout"] == "30" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Missing test coverage for non-UTF-8 response decoding.
Given the gap flagged in public_json_fetch.py (Lines 29-39) where UnicodeDecodeError isn't wrapped, consider adding a test with a non-UTF-8 byte payload (e.g., io.BytesIO(b"\xff\xfe")) asserting it raises PublicJsonError once that fix lands.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_public_json_fetch.py` around lines 22 - 40, Add coverage for the
non-UTF-8 decode path in load_public_json by introducing a test that uses a fake
response body with invalid UTF-8 bytes (for example, via the existing
FakeResponse in test_load_public_json_sets_default_headers) and asserts the call
raises PublicJsonError instead of leaking UnicodeDecodeError. Keep the test
alongside the current urllib.request.urlopen patching so it exercises the same
public_json_fetch behavior and verifies the new error wrapping once implemented.
| class FakeResponse(io.BytesIO): | ||
| def __init__(self) -> None: | ||
| super().__init__(b"[]") | ||
|
|
||
| def fake_urlopen(request, timeout=30): # noqa: ANN001 | ||
| captured["accept"] = request.get_header("Accept") | ||
| captured["user_agent"] = request.get_header("User-agent") | ||
| captured["timeout"] = str(timeout) | ||
| return FakeResponse() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Duplicate FakeResponse class across tests.
FakeResponse is redefined almost identically in test_load_public_json_sets_default_headers and test_load_public_json_honors_custom_timeout. Consider extracting a small module-level helper/fixture to avoid the duplication.
Also applies to: 82-88
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_public_json_fetch.py` around lines 25 - 33, The two tests are
duplicating the same FakeResponse helper, so extract that shared response stub
into a single module-level helper or fixture and reuse it from both
test_load_public_json_sets_default_headers and
test_load_public_json_honors_custom_timeout. Keep the behavior of FakeResponse
the same (the bytes-backed response object used by fake_urlopen), and update
both test functions to reference the shared helper instead of redefining it.
qingfeng312
left a comment
qingfeng312
left a comment
There was a problem hiding this comment.
Follow-up on current head b9293e7f507f6d2dee55478773b40fb006b86fd7.
Approved. The previous CRLF/trailing-whitespace blocker is resolved: the current patch no longer carries carriage-return bytes in added lines, and the hosted quality/readiness check is green.
I rechecked the shared public JSON helper, claim inventory integration, safety-cap guards, and tests. The helper preserves explicit Accept/User-Agent headers, timeout handling, HTTP/URL/JSON decode error wrapping, and the inventory script now fails closed when live public/GitHub collections hit the configured safety caps.
|
@qingfeng312 CI is green on the latest head ( |
|
@qingfeng312 — CI fully green on latest head for bounty #1144. PR #1174 (
RE-APPROVED on prior head — please recheck latest when convenient. Merge-ready. Wallet: |
|
@qingfeng312 — CI fully green on Wallet: |
3 similar comments
|
@qingfeng312 — CI fully green on Wallet: |
|
@qingfeng312 — CI fully green on Wallet: |
|
@qingfeng312 — CI fully green on Wallet: |
|
@qingfeng312 CI is green on the latest head ( |
|
Cash-out merge-ready follow-up — @qingfeng312 CI fully green on Wallet: |
|
@qingfeng312 Thanks for the review — pushed LF-normalized line endings for Head should now pass Wallet: |
yanyishuai
force-pushed
the
fix/issue-1144-shared-json-fetch
branch
from
b9293e7 to
6430616
Compare
3 participants
Summary
Adds
scripts/public_json_fetch.pyexposing oneload_public_jsonhelper plus aPublicJsonErrorfor read-only public JSON fetches. Migratesscripts/claim_inventory.pyto call the new helper while preserving its existing_get_jsonwrapper (callers see no behaviour change). Addstests/test_public_json_fetch.pycovering the happy path, default headers, HTTP error, timeout, invalid JSON, and custom timeout.Why
Five maintenance scripts (
claim_inventory,submission_quality_gate,proposed_work_triage,check_bounty_issue_states,check_live_bounty_closing_refs) each implemented their own small public-JSON fetch helper aroundurllib.request.urlopen. They did not agree onAcceptheaders, timeout values, or error wording. A future bugfix to any of those details had to be copy-pasted across files and the error wording silently diverged. This PR ships the helper plus one concrete migration so the contract is proven on a real caller; the remaining scripts can be migrated in follow-up PRs.Behavior
load_public_json(url, description=None, timeout=30, user_agent="mergework-maintenance-script", accept="application/json") -> Any.PublicJsonErrorraised on HTTP error, network/timeout failure, or invalid JSON, with a uniform "{description} unavailable: ..." message.scripts/claim_inventory.py::_get_jsonnow delegates toload_public_jsonand threads the existingGH_TIMEOUT_SECONDSconstant.Tests
tests/test_public_json_fetch.py(6 cases).Out of scope
Closes #1144
Solana wallet for bounty payout:
Do4v7foHJvRJLpRRoGaVPWX6DDEjX3yTK7J91gpwUQpESummary by CodeRabbit
New Features
Bug Fixes
Tests