Fix: honor offset parameter on bounty list endpoints#575
Conversation
The public API and MCP list_bounties tool accepted limit but ignored offset, making pagination impossible for clients that need to page through results. Changes: - app/bounty_api.py: add offset to _list_bounties_by_status, api_bounties, and api_bounties_summary - app/public_routes.py: pass limit/offset through to the bounties HTML page - app/mcp_tools.py: add list_offset_arg() and apply offset to MCP list_bounties - tests/test_bounty_offset.py: regression tests for API, summary, public page, MCP tool, and negative-offset rejection Validation: 441 passed, ruff check/format ok, mypy app ok.
📝 WalkthroughWalkthroughFour files implement offset-based pagination (parameter bounds 0–10,000) across REST API ( ChangesOffset Pagination Implementation
Possibly Related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6023726f-781c-4d66-ba2f-752b20b98c4f
📒 Files selected for processing (4)
app/bounty_api.pyapp/mcp_tools.pyapp/public_routes.pytests/test_bounty_offset.py
| def list_offset_arg() -> int: | ||
| if "offset" not in args or args.get("offset") is None: | ||
| return 0 | ||
| value = positive_int_arg("offset") | ||
| if value > 10_000: | ||
| raise ValueError("offset must be at most 10000") | ||
| return value |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Test that MCP list_bounties rejects offset=0 before fix
python -c "
import sys
import json
from app.mcp_tools import call_mcp_tool
from app.db import session_scope
from app.ledger.service import ensure_genesis, create_bounty
from tests.conftest import create_schema
# Use in-memory SQLite for quick test
db_url = 'sqlite:///:memory:'
create_schema(db_url)
with session_scope(db_url) as session:
ensure_genesis(session)
create_bounty(
session,
repo='test/repo',
issue_number=1,
issue_url='https://github.com/test/repo/issues/1',
title='Test',
reward_mrwk='10',
acceptance='Test'
)
try:
result = call_mcp_tool(db_url, 'list_bounties', {'status': 'open', 'offset': 0})
print('PASS: offset=0 accepted')
sys.exit(0)
except ValueError as e:
if 'must be positive' in str(e):
print(f'FAIL: offset=0 rejected with: {e}')
sys.exit(1)
raise
"Repository: ramimbo/mergework
Length of output: 296
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show relevant helper implementations and list_offset_arg usage
echo "---- app/mcp_tools.py (list_offset_arg + helpers) ----"
python - <<'PY'
import itertools, pathlib
path = pathlib.Path("app/mcp_tools.py")
lines = path.read_text().splitlines()
def show(start,end):
for i in range(start-1,end):
if i<0 or i>=len(lines): continue
print(f"{i+1:4d}: {lines[i]}")
# likely ranges; adjust if file shorter
show(1,120)
print("\n---- (continue) ----\n")
show(120,220)
PY
# Locate REST API offset validation
echo "---- bounty_api.py (offset Query bounds) ----"
python - <<'PY'
import pathlib, re
path = pathlib.Path("app/bounty_api.py")
if not path.exists():
print("File app/bounty_api.py not found")
raise SystemExit(0)
text = path.read_text().splitlines()
for i,line in enumerate(text,1):
if "offset" in line and "Query" in line:
print(f"{i:4d}: {line}")
PY
# Show the test that asserts offset=0 is accepted
echo "---- tests/test_bounty_offset.py ----"
python - <<'PY'
import pathlib
path = pathlib.Path("tests/test_bounty_offset.py")
if not path.exists():
print("File tests/test_bounty_offset.py not found")
raise SystemExit(0)
lines = path.read_text().splitlines()
for i in range(1,120):
if i<=len(lines):
print(f"{i:4d}: {lines[i-1]}")
PY
# Find positive_int_arg / int_arg definitions (if separate)
echo "---- Locate positive_int_arg and int_arg definitions ----"
rg -n "def (positive_int_arg|int_arg)\b" app/mcp_tools.py app -S || true
# Ensure where positive_int_arg is used
echo "---- Usages of positive_int_arg( ----"
rg -n "positive_int_arg\(" app -S || trueRepository: ramimbo/mergework
Length of output: 16728
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- tests/test_bounty_offset.py (continue from where we left off) ----"
python - <<'PY'
import pathlib
path = pathlib.Path("tests/test_bounty_offset.py")
lines = path.read_text().splitlines()
start=100
end=220
for i in range(start, min(end, len(lines))):
print(f"{i+1:4d}: {lines[i]}")
PYRepository: ramimbo/mergework
Length of output: 1619
Critical: MCP list_bounties rejects offset=0 while the REST API allows it
app/mcp_tools.py:list_offset_arg() calls positive_int_arg("offset"), which raises for value <= 0 (“offset must be positive”), so passing offset=0 fails. The REST endpoints allow offset=0 via Query(ge=0, le=10000), and tests/test_bounty_offset.py asserts ...&offset=0 returns the first page.
🐛 Proposed fix
def list_offset_arg() -> int:
if "offset" not in args or args.get("offset") is None:
return 0
- value = positive_int_arg("offset")
+ value = int_arg("offset")
+ if value < 0:
+ raise ValueError("offset must be non-negative")
if value > 10_000:
raise ValueError("offset must be at most 10000")
return valueAdd/extend an MCP test to cover offset=0 parity with the REST API.
📝 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 list_offset_arg() -> int: | |
| if "offset" not in args or args.get("offset") is None: | |
| return 0 | |
| value = positive_int_arg("offset") | |
| if value > 10_000: | |
| raise ValueError("offset must be at most 10000") | |
| return value | |
| def list_offset_arg() -> int: | |
| if "offset" not in args or args.get("offset") is None: | |
| return 0 | |
| value = int_arg("offset") | |
| if value < 0: | |
| raise ValueError("offset must be non-negative") | |
| if value > 10_000: | |
| raise ValueError("offset must be at most 10000") | |
| return value |
| sort = normalize_bounty_sort(optional_clean_str_arg("sort")) | ||
| limit = list_limit_arg() | ||
| offset = list_offset_arg() | ||
| if sort == "newest": | ||
| newest_bounties = session.scalars( | ||
| query.order_by(Bounty.id.desc()).limit(limit) | ||
| query.order_by(Bounty.id.desc()).offset(offset).limit(limit) | ||
| ).all() | ||
| return json.dumps([bounty_to_dict(bounty) for bounty in newest_bounties]) | ||
| bounties = session.scalars(query.order_by(Bounty.id.desc())).all() | ||
| sorted_bounties = sort_bounties([bounty_to_dict(bounty) for bounty in bounties], sort) | ||
| return json.dumps(sorted_bounties[:limit]) | ||
| start = offset | ||
| end = start + limit | ||
| return json.dumps(sorted_bounties[start:end]) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify code quality and type checking per guidelines
ruff format --check app/mcp_tools.py
ruff check app/mcp_tools.py
mypy app/mcp_tools.pyRepository: ramimbo/mergework
Length of output: 179
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ruff format --check .
ruff check .
if ! command -v mypy >/dev/null 2>&1; then
python3 -m pip install -q mypy
fi
mypy appRepository: ramimbo/mergework
Length of output: 429
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ruff format --check .
ruff check .
# Ensure mypy is installed, then run it without relying on PATH
python3 -m pip show mypy >/dev/null 2>&1 || python3 -m pip install -q mypy
python3 -m mypy appRepository: ramimbo/mergework
Length of output: 14097
Ruff checks pass, but mypy app fails (type-checking compliance blocker).
ruff format --check .andruff check .both pass.mypy appfails with many errors, includingapp/mcp_tools.py:6(“Cannot find implementation or library stub for module named "sqlalchemy"”), plus widespread FastAPI/SQLAlchemy missing-import issues and other mypy failures elsewhere—so the change doesn’t meet the “mypy app” guideline yet.
| limit: int | None = Query(None), | ||
| offset: int | None = Query(None), |
There was a problem hiding this comment.
Consider adding validation bounds for limit and offset.
The REST API endpoints validate limit: Query(ge=1, le=200) and offset: Query(ge=0, le=10000) (bounty_api.py:136-137, 146-147), but the public route accepts any integer without bounds.
This allows unexpected inputs like negative offset (uses Python negative indexing) or extremely large values, creating inconsistency across entry points.
📋 Suggested bounds for consistency
- limit: int | None = Query(None),
- offset: int | None = Query(None),
+ limit: int | None = Query(None, ge=1, le=200),
+ offset: int | None = Query(None, ge=0, le=10000),📝 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.
| limit: int | None = Query(None), | |
| offset: int | None = Query(None), | |
| limit: int | None = Query(None, ge=1, le=200), | |
| offset: int | None = Query(None, ge=0, le=10000), |
| for i in range(3): | ||
| create_bounty( | ||
| session, | ||
| repo="ramimbo/mergework", | ||
| issue_number=200 + i, | ||
| issue_url=f"https://github.com/ramimbo/mergework/issues/{200 + i}", | ||
| title=f"Summary bounty {i}", | ||
| reward_mrwk="20", | ||
| acceptance="Test acceptance.", |
There was a problem hiding this comment.
Summary pagination assertion doesn’t actually prove offset is honored.
On Line 72, asserting only bounties_shown == 1 can still pass if offset is ignored and only limit=1 is applied. Make seeded bounties value-distinct and assert an offset-sensitive aggregate (for example open_pool_mrwk) differs between offset 0 and 1.
Proposed test hardening
@@
- for i in range(3):
+ for i in range(3):
create_bounty(
session,
repo="ramimbo/mergework",
issue_number=200 + i,
issue_url=f"https://github.com/ramimbo/mergework/issues/{200 + i}",
title=f"Summary bounty {i}",
- reward_mrwk="20",
+ reward_mrwk=str(20 + i),
acceptance="Test acceptance.",
)
@@
- full = client.get("/api/v1/bounties/summary").json()
- assert full["bounties_shown"] == 3
-
- limited = client.get("/api/v1/bounties/summary?limit=1&offset=1").json()
- assert limited["bounties_shown"] == 1
+ full = client.get("/api/v1/bounties/summary").json()
+ assert full["bounties_shown"] == 3
+
+ first_page = client.get("/api/v1/bounties/summary?limit=1&offset=0").json()
+ second_page = client.get("/api/v1/bounties/summary?limit=1&offset=1").json()
+ assert first_page["bounties_shown"] == 1
+ assert second_page["bounties_shown"] == 1
+ assert first_page["open_pool_mrwk"] != second_page["open_pool_mrwk"]Also applies to: 69-73
| result = call_mcp_tool(sqlite_url, "list_bounties", {"status": "open", "limit": 2, "offset": 1}) | ||
| bounties = json.loads(result) | ||
| assert len(bounties) == 2 | ||
| assert bounties[0]["title"] == "MCP bounty 2" | ||
|
|
There was a problem hiding this comment.
MCP pagination test should assert the full returned window, not just the first row.
Line 124 checks only index 0; a broken second item/order could still pass. Assert both titles (or the whole title list) for the expected slice.
Proposed assertion tightening
result = call_mcp_tool(sqlite_url, "list_bounties", {"status": "open", "limit": 2, "offset": 1})
bounties = json.loads(result)
assert len(bounties) == 2
- assert bounties[0]["title"] == "MCP bounty 2"
+ assert [b["title"] for b in bounties] == ["MCP bounty 2", "MCP bounty 1"]
akmhatey-ai
left a comment
akmhatey-ai
left a comment
There was a problem hiding this comment.
I independently checked current head f82642d587f38adc5bb9bc3b58604d38c41f9bef and found boundary issues that should be fixed before merge.
The API route and MCP/public route contracts are inconsistent:
/api/v1/bounties?offset=0returns HTTP 200, but MCPlist_bountieswith{"status":"open","limit":1,"offset":0}raisesValueError: offset must be positive. Missing MCP offset defaults to0, so explicit zero should not fail.- The public
/bountiesroute declareslimitandoffsetas plain optional ints, so invalid values are accepted and passed into Python slicing. Local repros on this head:/bounties?offset=-1-> HTTP 200/bounties?limit=-1-> HTTP 200 and omits the oldest row because[: -1]is applied/bounties?limit=0-> HTTP 200 with an empty page/bounties?limit=10001-> HTTP 200
- The REST API rejects the same invalid cases as expected:
/api/v1/bounties?offset=-1,limit=-1,limit=0, andlimit=10001all return 422.
Suggested fix: allow explicit MCP offset=0, and give the public route the same lower/upper bounds as the REST route before passing values into the shared list helper.
Validation I ran locally:
python -m pytest tests\test_bounty_offset.py -q->5 passedpython -m ruff check app\bounty_api.py app\mcp_tools.py app\public_routes.py tests\test_bounty_offset.py-> passedpython -m ruff format --check app\bounty_api.py app\mcp_tools.py app\public_routes.py tests\test_bounty_offset.py->4 files already formattedpython -m mypy app\bounty_api.py app\mcp_tools.py app\public_routes.py-> successgit diff --check origin/main...HEAD-> clean- diff-only Gitleaks scan -> no leaks
No private data, credentials, wallet material, signatures, production mutation, price/off-ramp, exchange/liquidity, or fabricated payout claims were used.
eliasx45
left a comment
eliasx45
left a comment
There was a problem hiding this comment.
Reviewed current head f82642d587f38adc5bb9bc3b58604d38c41f9bef.
Thanks for adding offset coverage across the bounty surfaces. I need to request changes because the public and MCP validation contracts diverge from the REST API in ways that are visible to callers.
Reproduced locally with three open bounties:
/api/v1/bounties?offset=-1 -> 422
/bounties?offset=-1 -> 200
/api/v1/bounties?limit=-1 -> 422
/bounties?limit=-1 -> 200, and Python slicing omits the oldest row
/api/v1/bounties?limit=0 -> 422
/bounties?limit=0 -> 200, empty page
/api/v1/bounties?limit=10001 -> 422
/bounties?limit=10001 -> 200
The public route currently declares limit: int | None = Query(None) and offset: int | None = Query(None), so invalid values are accepted and passed into slicing. It should use the same bounds as the REST route before calling the shared helper.
There is also an MCP zero-boundary mismatch:
call_mcp_tool(..., "list_bounties", {"status":"open","limit":1,"offset":0})
-> ValueError: offset must be positive
call_mcp_tool(..., "list_bounties", {"status":"open","limit":1})
-> succeeds, default offset is 0
Since REST accepts offset=0 and the MCP default is also zero, explicit MCP offset: 0 should not fail. Suggested fix: validate MCP offset with a non-negative bound, and give public /bounties the same limit 1..200 and offset 0..10000 FastAPI constraints as the REST endpoints.
Validation run locally:
pytest tests\\test_bounty_offset.py -q-> 5 passedruff check app\\bounty_api.py app\\mcp_tools.py app\\public_routes.py tests\\test_bounty_offset.py-> passedruff format --check app\\bounty_api.py app\\mcp_tools.py app\\public_routes.py tests\\test_bounty_offset.py-> 4 files already formattedmypy app\\bounty_api.py app\\mcp_tools.py app\\public_routes.py-> successscripts\\docs_smoke.py-> docs smoke okgit diff --check origin/main...HEAD-> clean
|
Maintainer cleanup note: holding this for changes. Reviews found the offset/limit contract still diverges across REST, public, and MCP paths, including invalid public |
4 participants
Closes #406
The public API and MCP
list_bountiestool acceptedlimitbut ignoredoffset, making pagination impossible for clients that need to page through results.Changes:
app/bounty_api.py: addoffsetto_list_bounties_by_status,api_bounties, andapi_bounties_summaryapp/public_routes.py: passlimit/offsetthrough to the bounties HTML pageapp/mcp_tools.py: addlist_offset_arg()and applyoffsetto MCPlist_bountiestests/test_bounty_offset.py: regression tests for API, summary, public page, MCP tool, and negative-offset rejectionValidation:
Summary by CodeRabbit
offsetquery parameter (0–10,000 range) across API endpoints and the web interface for improved browsing of bounties.