Refs #406: Support offset on bounty attempts API#541
Conversation
📝 WalkthroughWalkthroughAdds an integer ChangesPagination Support for Bounty Attempts
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
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: 71bcaa32-6461-49c6-8101-adb704247d0e
📒 Files selected for processing (2)
app/bounty_attempts.pytests/test_bounty_attempts.py
|
Follow-up for CodeRabbit discussion #541 (comment): Updated in commit Validation after update:
|
weilixiong
left a comment
There was a problem hiding this comment.
APPROVED — Clean addition of offset parameter to bounty attempts API. +22/-2, proper Query validation (ge=0), test covers offset=1 and negative offset (422). Well-scoped.
adliebe
left a comment
There was a problem hiding this comment.
Reviewed PR #541 at current head 330d1d040183e2409e469daab1fb5e94c862c12d as non-author adliebe.
I do not see a blocker in this slice.
Evidence:
- Inspected
app/bounty_attempts.pyandtests/test_bounty_attempts.py. - Verified the API adds
offset: int = Query(0, ge=0)toGET /api/v1/bounties/{bounty_id}/attempts, so negative offsets are rejected by request validation before query execution. - Verified
list_bounty_attempts()applies the offset after the same bounty/status/expiry filtering and deterministiccreated_at desc, id descordering used by the existing attempts listing. - Verified the new regression covers the important visible behaviors:
offset=1skips the newest active attempt,offset=-1returns 422, and an out-of-range offset returns HTTP 200 with an emptyattemptslist. - Checked visible GitHub state before posting: PR #541 is open, non-draft,
CLEAN, and both Quality/readiness/docs/image checks plus CodeRabbit are successful. - Checked bounty #447 comments before posting and found no exact visible
pull/541,PR #541, or#541 (review)claim match.
Validation:
..\mergework\.venv\Scripts\python.exe -m pytest tests/test_bounty_attempts.py -qpassed, 7 tests...\mergework\.venv\Scripts\ruff.exe check app\bounty_attempts.py tests\test_bounty_attempts.pypassed...\mergework\.venv\Scripts\ruff.exe format --check app\bounty_attempts.py tests\test_bounty_attempts.pypassed...\mergework\.venv\Scripts\python.exe scripts\docs_smoke.pypassed...\mergework\.venv\Scripts\python.exe -m mypy app\bounty_attempts.pypassed.git diff --check d8532d4d909f47c689d3b88eb21461243097673f...HEADpassed.
Verdict: approved. The offset behavior is scoped to public read pagination, keeps the existing warning and filtering contract, and does not add payout guarantees, private account material, wallet secrets, liquidity, exchange, or off-ramp claims.
tolga-tom-nook
left a comment
There was a problem hiding this comment.
Reviewed PR #541 at current head 330d1d040183e2409e469daab1fb5e94c862c12d as a non-author for the rolling review bounty (#447).
Verdict: no blockers found for this focused API/test change.
Evidence:
- Inspected
app/bounty_attempts.py: the newoffset: int = Query(0, ge=0)is only exposed onGET /api/v1/bounties/{bounty_id}/attempts, is passed intolist_bounty_attempts, and is applied after the existing active/expired filtering plus newest-first ordering. That preserves the current default behavior whenoffset=0and makes pagination semantics deterministic. - Inspected
tests/test_bounty_attempts.py: coverage now checks the useful page boundary (offset=1skips the newest visible attempt), validation (offset=-1returns 422), and out-of-range behavior (offset=99returns an empty list). This matches the endpoint contract without changing attempt creation/release behavior. - Ran locally from the PR branch:
python3 -m pytest tests/test_bounty_attempts.py -q->7 passed in 2.82s. - Ran locally from the PR branch:
python3 -m ruff check app/bounty_attempts.py tests/test_bounty_attempts.py->All checks passed!.
Compatibility/risk notes: this is a small additive query parameter, so existing clients that omit offset continue to receive the same newest-first active attempt list. I do not see a merge blocker.
Baijack-star
left a comment
There was a problem hiding this comment.
Detailed review for the bounty-attempts offset change. I found one blocking input-boundary issue.
Finding:
offsetis only constrained withge=0, then passed directly into SQLAlchemy.offset(offset). Very large positive integers parse as Python ints and can reach SQLite as an OFFSET bind value, which raisesOverflowError: Python int too large to convert to SQLite INTEGERinstead of returning a bounded 4xx response. This is the same public-integer-boundary class the repo already guards for path IDs/search issue numbers.
Reproduction on this PR branch:
GET /api/v1/bounties/{id}/attempts?offset=9999999999999999999999999999999999999999
-> OverflowError: Python int too large to convert to SQLite INTEGER
GET /api/v1/bounties/{id}/attempts?offset=9223372036854775808
-> OverflowError: Python int too large to convert to SQLite INTEGER
GET /api/v1/bounties/{id}/attempts?offset=9223372036854775807
-> 200 {"bounty_id":1,"warnings":[],"attempts":[]}
Suggested fix: bound the query parameter before it reaches SQLAlchemy, e.g. Query(0, ge=0, le=SQLITE_INTEGER_MAX) using the existing constant from app.path_params, and add a regression for one oversized offset returning 422.
Validation also run:
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_bounty_attempts.py -q-> 7 passeduv run --extra dev ruff check app/bounty_attempts.py tests/test_bounty_attempts.py-> passed
No wallet material, private keys, tokens, private data, production mutation, price/off-ramp claims, or fabricated payout claims were used.
|
TinyOps Studio LLC reviewed PR #541 at Evidence checked:
Validation run locally from an isolated worktree:
Assessment: no blocker found. The change is a narrow pagination addition with the important negative and empty-page cases covered. |
eliasx45
left a comment
There was a problem hiding this comment.
Reviewed PR #541 at head 330d1d040183e2409e469daab1fb5e94c862c12d for the bounty-attempts offset pagination change.
The focused behavior works for normal bounds: offset=1 skips the newest active attempt, offset=-1 is rejected by FastAPI validation, and an out-of-range but SQLite-sized offset returns an empty attempt list.
Blocking issue:
offsetis only constrained withQuery(0, ge=0), then passed directly to SQLAlchemy.offset(offset). Oversized positive integers parse as Pythonintand reach SQLite, where they raiseOverflowErrorinstead of returning a bounded validation error.- Reproduced locally on this branch:
GET /api/v1/bounties/{id}/attempts?offset=9223372036854775807->200 {"attempts": []}GET /api/v1/bounties/{id}/attempts?offset=9223372036854775808->OverflowError: Python int too large to convert to SQLite INTEGERGET /api/v1/bounties/{id}/attempts?offset=9999999999999999999999999999999999999999-> sameOverflowError
- Please bound
offsetbefore it reaches SQLAlchemy, for example with the existing SQLite integer max boundary, and add a regression asserting the oversized value returns 422.
Validation run locally:
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 .\.venv\Scripts\python.exe -m pytest tests\test_bounty_attempts.py -q-> 7 passed.\.venv\Scripts\python.exe -m ruff check app\bounty_attempts.py tests\test_bounty_attempts.py-> all checks passed.\.venv\Scripts\python.exe -m ruff format --check app\bounty_attempts.py tests\test_bounty_attempts.py-> 2 files already formattedPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 .\.venv\Scripts\python.exe -m mypy app\bounty_attempts.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 unbounded public integer input.
|
Maintainer cleanup note: holding this for changes. The review found oversized positive |
Summary
offsetquery parameter toGET /api/v1/bounties/{id}/attemptsoffset=1skips the newest active attempt,offset=-1is rejected, and an out-of-range offset returns an empty attempts listEvidence
limit, while this PR covers the distinctoffsetbehavior.app/bounty_attempts.pyandtests/test_bounty_attempts.pyonly.Test Evidence
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_bounty_attempts.py -q(7 passed)uv run --extra dev ruff check app/bounty_attempts.py tests/test_bounty_attempts.pyuv run --extra dev ruff format --check app/bounty_attempts.py tests/test_bounty_attempts.pyPYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m mypy app/bounty_attempts.pygit diff --checkBounty
Refs #406
Public Artifact Hygiene
No private data, wallet material, signatures, payout claims, price claims, off-ramp claims, or security-sensitive details are included.