Skip to content

Refs #406: Support offset on bounty attempts API#541

Open
Juanpablo24-06 wants to merge 2 commits into
ramimbo:mainfrom
Juanpablo24-06:codex/b406-attempts-offset
Open

Refs #406: Support offset on bounty attempts API#541
Juanpablo24-06 wants to merge 2 commits into
ramimbo:mainfrom
Juanpablo24-06:codex/b406-attempts-offset

Conversation

@Juanpablo24-06
Copy link
Copy Markdown
Contributor

@Juanpablo24-06 Juanpablo24-06 commented May 27, 2026

Summary

  • add a validated offset query parameter to GET /api/v1/bounties/{id}/attempts
  • apply the offset after the existing newest-first ordering and active/expired filtering
  • add regression coverage showing offset=1 skips the newest active attempt, offset=-1 is rejected, and an out-of-range offset returns an empty attempts list

Evidence

  • Confusion, missing step, stale example, or bug this addresses: public list endpoints are gaining pagination, but the bounty-attempt coordination endpoint only exposed the first ordered slice; clients could not skip already-seen attempt rows.
  • Bounty capacity and active attempts/open PRs checked: bounty MRWK bounty: useful bug reports and small fixes, round 5 #406 / internal bounty 66 was open with awards remaining when the PR was opened; duplicate search found PR Add limit to bounty attempts list #491 covering attempt limit, while this PR covers the distinct offset behavior.
  • Intended files or surfaces: app/bounty_attempts.py and tests/test_bounty_attempts.py only.
  • Expected PR size: small, focused API/test patch.
  • Out of scope: no attempt creation changes, no authentication changes, no payout or ledger changes, no docs rewrite.

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.py
  • uv run --extra dev ruff format --check app/bounty_attempts.py tests/test_bounty_attempts.py
  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m mypy app/bounty_attempts.py
  • git diff --check

Bounty

Refs #406

Public Artifact Hygiene

No private data, wallet material, signatures, payout claims, price claims, off-ramp claims, or security-sensitive details are included.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 27, 2026

📝 Walkthrough

Walkthrough

Adds an integer offset query parameter to the bounty-attempts GET endpoint and the list_bounty_attempts helper; the helper applies SQL OFFSET when set. Tests verify valid and invalid offsets and empty-page behavior.

Changes

Pagination Support for Bounty Attempts

Layer / File(s) Summary
Offset pagination in list_bounty_attempts and API endpoint
app/bounty_attempts.py
list_bounty_attempts() accepts an offset: int = 0 and applies SQL OFFSET after ordering when non-zero. The GET /api/v1/bounties/{bounty_id}/attempts handler adds offset: int = Query(0, ge=0) and forwards it to the helper.
Pagination test coverage
tests/test_bounty_attempts.py
Tests call the endpoint with offset=1 and assert only the second attempt is returned; they also assert offset=-1 yields HTTP 422 and a large offset returns HTTP 200 with an empty attempts list.
  • Possibly related PRs:
    • ramimbo/mergework#337: Prior work introducing the list_bounty_attempts helper that this change extends with offset pagination.
🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Title check ✅ Passed The title directly names the changed surface (offset on bounty attempts API) and references the bounty issue (#406), meeting the requirement for a short, concrete title.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Mergework Public Artifact Hygiene ✅ Passed PR is purely technical pagination feature; no investment, price, cash-out, payout, or security claims found in code, tests, or PR description.
Bounty Pr Focus ✅ Passed PR changes match stated files, offset parameter validated and applied after filtering/ordering, test coverage confirms bounds validation and correct skipping behavior.
Description check ✅ Passed The pull request description provides complete coverage of all required template sections with specific details about the offset feature, evidence, test validation, bounty reference, and scope boundaries.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between d8532d4 and f63f0e2.

📒 Files selected for processing (2)
  • app/bounty_attempts.py
  • tests/test_bounty_attempts.py

Comment thread tests/test_bounty_attempts.py
@Juanpablo24-06
Copy link
Copy Markdown
Contributor Author

Follow-up for CodeRabbit discussion #541 (comment):

Updated in commit 330d1d0: the attempts offset regression now also asserts offset=-1 is rejected with 422 and an out-of-range offset returns an empty attempts list.

Validation after update:

  • 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 tests/test_bounty_attempts.py
  • uv run --extra dev ruff format --check tests/test_bounty_attempts.py
  • git diff --check

Copy link
Copy Markdown

@weilixiong weilixiong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@adliebe adliebe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.py and tests/test_bounty_attempts.py.
  • Verified the API adds offset: int = Query(0, ge=0) to GET /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 deterministic created_at desc, id desc ordering used by the existing attempts listing.
  • Verified the new regression covers the important visible behaviors: offset=1 skips the newest active attempt, offset=-1 returns 422, and an out-of-range offset returns HTTP 200 with an empty attempts list.
  • 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 -q passed, 7 tests.
  • ..\mergework\.venv\Scripts\ruff.exe check app\bounty_attempts.py tests\test_bounty_attempts.py passed.
  • ..\mergework\.venv\Scripts\ruff.exe format --check app\bounty_attempts.py tests\test_bounty_attempts.py passed.
  • ..\mergework\.venv\Scripts\python.exe scripts\docs_smoke.py passed.
  • ..\mergework\.venv\Scripts\python.exe -m mypy app\bounty_attempts.py passed.
  • git diff --check d8532d4d909f47c689d3b88eb21461243097673f...HEAD passed.

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.

Copy link
Copy Markdown
Contributor

@tolga-tom-nook tolga-tom-nook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 new offset: int = Query(0, ge=0) is only exposed on GET /api/v1/bounties/{bounty_id}/attempts, is passed into list_bounty_attempts, and is applied after the existing active/expired filtering plus newest-first ordering. That preserves the current default behavior when offset=0 and makes pagination semantics deterministic.
  • Inspected tests/test_bounty_attempts.py: coverage now checks the useful page boundary (offset=1 skips the newest visible attempt), validation (offset=-1 returns 422), and out-of-range behavior (offset=99 returns 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.

Copy link
Copy Markdown

@Baijack-star Baijack-star left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Detailed review for the bounty-attempts offset change. I found one blocking input-boundary issue.

Finding:

  • offset is only constrained with ge=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 raises OverflowError: Python int too large to convert to SQLite INTEGER instead 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 passed
  • uv 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.

@peterxing
Copy link
Copy Markdown

TinyOps Studio LLC reviewed PR #541 at 330d1d040183e2409e469daab1fb5e94c862c12d.

Evidence checked:

  • inspected app/bounty_attempts.py and tests/test_bounty_attempts.py;
  • confirmed the public attempts endpoint validates offset with Query(0, ge=0) and passes it into list_bounty_attempts without changing attempt creation, release, auth, payout, or ledger paths;
  • confirmed list_bounty_attempts applies the offset after active/expired filtering and newest-first ordering, before any limit, so offset=1 skips the newest visible attempt rather than changing the filter set;
  • confirmed regression coverage asserts the visible offset behavior, FastAPI rejection for offset=-1, and empty-page behavior for an out-of-range offset;
  • checked hosted PR state: open, non-draft, clean merge state, CodeRabbit success, and CI success.

Validation run locally from an isolated worktree:

  • 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.py -> passed
  • uv run --extra dev ruff format --check app/bounty_attempts.py tests/test_bounty_attempts.py -> 2 files already formatted
  • git diff --check origin/main...HEAD -> clean

Assessment: no blocker found. The change is a narrow pagination addition with the important negative and empty-page cases covered.

Copy link
Copy Markdown

@eliasx45 eliasx45 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  • offset is only constrained with Query(0, ge=0), then passed directly to SQLAlchemy .offset(offset). Oversized positive integers parse as Python int and reach SQLite, where they raise OverflowError instead 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 INTEGER
    • GET /api/v1/bounties/{id}/attempts?offset=9999999999999999999999999999999999999999 -> same OverflowError
  • Please bound offset before 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 formatted
  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 .\.venv\Scripts\python.exe -m mypy app\bounty_attempts.py -> success
  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 .\.venv\Scripts\python.exe scripts\docs_smoke.py -> docs smoke ok
  • git diff --check origin/main...HEAD -> clean

Requesting changes for the unbounded public integer input.

@ramimbo
Copy link
Copy Markdown
Owner

ramimbo commented May 28, 2026

Maintainer cleanup note: holding this for changes. The review found oversized positive offset values can still reach SQLite and raise OverflowError instead of returning a bounded validation response. Please add the boundary guard and regression coverage before maintainer acceptance.

@ramimbo ramimbo added the mrwk:needs-info More information needed label May 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

mrwk:needs-info More information needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants