feat: implement MCP resource for public bounty board data #791#828
feat: implement MCP resource for public bounty board data #791#828Angelebeats wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughAdds MCP resource support: resource type/registry, JSON-RPC routing for ChangesMCP Resources Support
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 |
![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: 170d27d8-5e15-45af-bff6-690e833667be
📒 Files selected for processing (3)
app/main.pyapp/mcp.pyapp/mcp_tools.py
| def call_mcp_resource(database_url: str, uri: str) -> str: | ||
| if uri == "bounties://active": | ||
| with session_scope(database_url) as session: | ||
| query = select(Bounty).where(Bounty.status == "open").order_by(Bounty.id.desc()) | ||
| bounties = session.scalars(query).all() | ||
| return json.dumps(bounties_to_dict(bounties, session=session)) | ||
| raise ValueError("unknown resource") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add tests for the new resource handler.
No test covers call_mcp_resource (the bounties://active happy path and the ValueError("unknown resource") branch). As per coding guidelines: "Add or update tests for changed behavior".
Want me to draft the tests for both branches?
Unbounded query: bounties://active returns every open bounty with no cap.
Unlike the list_bounties tool (Lines 113-119, 155), this handler applies no limit. On a public, unauthenticated /mcp endpoint the response size and the per-row pending-proposal preload in bounties_to_dict both scale with the number of open bounties, so a large board yields a large payload and query cost on every read.
Proposed fix: bound the result set
def call_mcp_resource(database_url: str, uri: str) -> str:
if uri == "bounties://active":
with session_scope(database_url) as session:
- query = select(Bounty).where(Bounty.status == "open").order_by(Bounty.id.desc())
+ query = (
+ select(Bounty)
+ .where(Bounty.status == "open")
+ .order_by(Bounty.id.desc())
+ .limit(100)
+ )
bounties = session.scalars(query).all()
return json.dumps(bounties_to_dict(bounties, session=session))
raise ValueError("unknown resource")If unbounded output is intentional for this resource, consider exposing pagination instead of returning the full table.
📝 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 call_mcp_resource(database_url: str, uri: str) -> str: | |
| if uri == "bounties://active": | |
| with session_scope(database_url) as session: | |
| query = select(Bounty).where(Bounty.status == "open").order_by(Bounty.id.desc()) | |
| bounties = session.scalars(query).all() | |
| return json.dumps(bounties_to_dict(bounties, session=session)) | |
| raise ValueError("unknown resource") | |
| def call_mcp_resource(database_url: str, uri: str) -> str: | |
| if uri == "bounties://active": | |
| with session_scope(database_url) as session: | |
| query = ( | |
| select(Bounty) | |
| .where(Bounty.status == "open") | |
| .order_by(Bounty.id.desc()) | |
| .limit(100) | |
| ) | |
| bounties = session.scalars(query).all() | |
| return json.dumps(bounties_to_dict(bounties, session=session)) | |
| raise ValueError("unknown resource") |
keilogic
left a comment
keilogic
left a comment
There was a problem hiding this comment.
Reviewed current head 75dc5e60bda9f0ef507af2cce39c134a5e905a08 against base 12ca2f0510abce4f3a8b3d77ae8596f47fa5c74c.
I am requesting changes for two current-head blockers:
resources/readreadsparams.get("uri")before confirmingparamsis an object. I reproducedAttributeErrorfor JSON-RPC requests with"params": [],null, or a string, so the public/mcppath can 500 instead of returning a JSON-RPC-32602error. This should follow thetools/callparams validation pattern and have regression coverage.- The PR is not passing required quality checks. Hosted
Quality, readiness, docs, and image checksfails becauseruff format --check .would reformatapp/main.py,app/mcp.py, andapp/mcp_tools.py; local targeted Ruff check also reports import ordering and an E501 inapp/mcp.py, andgit diff --check origin/main...HEADreports a blank line at EOF inapp/mcp_tools.py.
Other validation:
pytest tests/test_mcp_tools.py tests/test_api_mcp.py -q->108 passed, 1 warningpython scripts/docs_smoke.py->docs smoke okgit merge-tree --write-tree origin/main HEAD-> clean tree6f01cd9ec2d619824747778e6e884419c6e27a2c- CodeRabbit is successful but has actionable comments on this same head.
I kept this review to public PR code, public GitHub checks, and local validation; no private data or payment/treasury/wallet execution was involved.
gshaowei6
left a comment
gshaowei6
left a comment
There was a problem hiding this comment.
Reviewed current head 75dc5e60bda9f0ef507af2cce39c134a5e905a08 against the #791 proposal. I am requesting changes for an acceptance-scope gap that is separate from the existing params/formatting blockers.
#791 asks for one machine-readable bounty-board routing payload: live claimable rows plus pending create-bounty opening windows, treasury capacity/source URLs, and guidance that opening-soon issues are not claimable yet. The current implementation exposes bounties://active, but call_mcp_resource() only queries Bounty.status == "open" and serializes bounties_to_dict(...). A direct /mcp resources/read probe on this head returns a top-level JSON list, not a board object, and the payload has no claimable_now, opening_soon, treasury capacity, or source URL fields.
Local evidence:
TMP/TEMPpointed inside the worktree,python -m pytest tests/test_mcp_tools.py tests/test_api_mcp.py -q-> 108 passed.- Direct MCP probe:
resource_uri=bounties://active,payload_type=list,has_claimable_now=False,has_opening_soon=False. rgfound no tests/docs for the new MCP resource and no use oftreasury_status/pending create-bounty data in the resource implementation.python scripts/docs_smoke.py-> docs smoke ok.git merge-tree --write-tree origin/main HEAD-> clean tree661b823d01b90517ee089553832ef394c6b81876.- Existing hygiene failures remain: targeted Ruff reports import ordering plus an E501,
ruff format --checkwould reformat the three touched files, andgit diff --checkreports a blank line at EOF inapp/mcp_tools.py.
Suggested direction: make the resource return the board shape from #791, backed by both open bounty rows and treasury pending-create/status data, with tests for at least one claimable row, one opening-soon row, and the no-pending-create case. The resource URI/name should also match the documented board semantics rather than only "active bounties".
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/mcp.py (1)
13-13: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winSerialize dict resource payloads before assigning
contents[].text.
MCPResourceHandlersays a resource may returndict[str, Any], but this branch copies that value straight intotext. That makes the response shape depend on the handler implementation and will break callers that expecttextto stay a JSON string. Either JSON-encode dict results here or narrowMCPResourceHandlertostronly.Proposed fix
- content = call_resource(database_url, uri) + content = call_resource(database_url, uri) + text = json.dumps(content) if isinstance(content, dict) else content return { "jsonrpc": "2.0", "id": response_id, "result": { - "contents": [{"uri": uri, "mimeType": "application/json", "text": content}] + "contents": [{"uri": uri, "mimeType": "application/json", "text": text}] }, }Also applies to: 144-150
app/mcp_tools.py (1)
168-176:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not truncate
list_bountiesbefore applying non-newest filters/sorts.This branch now takes the newest 100 rows first and only then runs
filter_bounties_by_availability(...)andsort_bounties(...). That makes requests likesort != "newest"oravailability != "all"incomplete: older matches can never appear, even when they should be in the toplimit. Keep the cap only on thenewest/allfast path, or push the requested sort/filter before the cap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e44c587a-5d57-4fa5-a1e5-bd743361577c
📒 Files selected for processing (10)
app/bounty_attempts.pyapp/github_bounty_board.pyapp/ledger/service.pyapp/mcp.pyapp/mcp_tools.pyapp/models.pyapp/serializers.pyapp/treasury.pyapp/treasury_executor.pytests/test_mcp_board.py
tudorian95
left a comment
tudorian95
left a comment
There was a problem hiding this comment.
Reviewed current head 2992dec56b156e61b73d450c76c8836ae568c5bd as a non-author.
I am requesting changes because the current head still has merge/hygiene blockers before maintainers can safely evaluate the resource behavior.
What I checked:
- Inspected the current diff across
app/mcp.py,app/mcp_tools.py,app/main.py, andtests/test_mcp_board.py. - Confirmed the happy-path MCP resource smoke tests now pass on this head.
- Rechecked current-main mergeability because GitHub reports
mergeStateStatus=DIRTY/mergeable=CONFLICTING.
Validation run in an isolated Docker container from this head:
uv run --extra dev pytest tests/test_mcp_board.py tests/test_api_mcp.py::test_mcp_tools_list_and_call -q-> 3 passed, 1 existing Starlette/httpx warning.uv run --extra dev ruff check app/main.py app/mcp.py app/mcp_tools.py tests/test_mcp_board.py-> reports unsorted/unformatted import blocks inapp/main.pyandtests/test_mcp_board.py.uv run --extra dev ruff format --check app/main.py app/mcp.py app/mcp_tools.py tests/test_mcp_board.py-> would reformatapp/main.pyandtests/test_mcp_board.py.uv run --extra dev mypy app/mcp.py app/mcp_tools.py-> success.uv run --extra dev python scripts/docs_smoke.py-> docs smoke ok.git diff --check origin/main...HEAD-> reports trailing whitespace intests/test_mcp_board.pylines 68 and 72.git merge-tree --write-tree origin/main HEAD-> reports a content conflict while auto-mergingapp/bounty_attempts.py.
Actionable next step: rebase/merge current origin/main, resolve the app/bounty_attempts.py conflict, then run Ruff format/fix on app/main.py and tests/test_mcp_board.py and remove the trailing whitespace before rerunning the focused MCP tests.
Scope note: I did not use private data, secrets, wallet material, payout execution, treasury mutation, ledger mutation, bridge/exchange/cash-out behavior, or MRWK price behavior. This review is limited to public PR code, mergeability, and local validation evidence.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/mcp.py (1)
13-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSerialize dict resource payloads before writing them into
text.
MCPResourceHandleron Line 13 allows handlers to returndict[str, Any], but this branch copiescontentinto thetextfield unchanged. A dict-valued resource would emit{"text": {...}}instead of JSON text, which breaks theapplication/jsonresponse contract.Proposed fix
try: content = call_resource(database_url, uri) + text = content if isinstance(content, str) else json.dumps(content) return { "jsonrpc": "2.0", "id": response_id, "result": { - "contents": [{"uri": uri, "mimeType": "application/json", "text": content}] + "contents": [{"uri": uri, "mimeType": "application/json", "text": text}] }, }Also applies to: 169-176
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 837bc907-384c-44c5-8602-ce1db2501ee6
📒 Files selected for processing (6)
app/bounty_attempts.pyapp/main.pyapp/mcp.pyapp/mcp_tools.pyapp/serializers.pytests/test_mcp_board.py
💤 Files with no reviewable changes (1)
- app/main.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/mcp.py (1)
13-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSerialize dict resource payloads before writing them into
text.
MCPResourceHandleron Line 13 allows handlers to returndict[str, Any], but this branch copiescontentinto thetextfield unchanged. A dict-valued resource would emit{"text": {...}}instead of JSON text, which breaks theapplication/jsonresponse contract.Proposed fix
try: content = call_resource(database_url, uri) + text = content if isinstance(content, str) else json.dumps(content) return { "jsonrpc": "2.0", "id": response_id, "result": { - "contents": [{"uri": uri, "mimeType": "application/json", "text": content}] + "contents": [{"uri": uri, "mimeType": "application/json", "text": text}] }, }Also applies to: 169-176
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 837bc907-384c-44c5-8602-ce1db2501ee6
📒 Files selected for processing (6)
app/bounty_attempts.pyapp/main.pyapp/mcp.pyapp/mcp_tools.pyapp/serializers.pytests/test_mcp_board.py
💤 Files with no reviewable changes (1)
- app/main.py
🛑 Comments failed to post (1)
app/mcp.py (1)
24-27:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the advertised tool contract in sync with the handlers.
These descriptions say
get_bountysupportsissue_number/repoandlist_bounty_attemptssupportsissue_number, but the implementations now only acceptidandbounty_idrespectively (see Line 177 and Line 185 inapp/mcp_tools.py). MCP clients discover argument semantics fromtools/list, so this will drive valid-looking calls intoinvalid tool arguments.Proposed fix
{ "name": "get_bounty", "description": ( - "Get a bounty by internal id, or by GitHub issue_number with optional repo, " - "optionally with accepted awards" + "Get a bounty by internal id, optionally with accepted awards" ), }, { "name": "list_bounty_attempts", "description": ( - "List advisory active-attempt reservations for a bounty by internal bounty_id, " - "or by GitHub issue_number with optional repo" + "List advisory active-attempt reservations for a bounty by internal bounty_id" ), },Also applies to: 31-34
xiefuzheng713-alt
left a comment
xiefuzheng713-alt
left a comment
There was a problem hiding this comment.
Requesting changes on current head 7e9674cecf41c548364cf0d38ea5ba3758ae0c2a.
I found current-head blockers in the MCP tool selector contract, separate from the resource happy-path tests:
get_bountyno longer accepts the advertisedissue_number+ optionalreposelector. The existing regressiontests/test_api_mcp.py::test_mcp_get_bounty_accepts_issue_number_selectornow returns a JSON-RPC error instead of a result.list_bounty_attemptssimilarly no longer acceptsissue_number+ optionalrepo;tests/test_api_mcp.py::test_mcp_list_bounty_attempts_accepts_issue_number_selectorfails withKeyError: 'result'because the response is an error.- Mixed
get_bountyselectors are no longer rejected.tests/test_api_mcp.py::test_mcp_get_bounty_rejects_mixed_selectorsexpects-32602 invalid tool arguments, but current head returns the bounty selected byid. - The branch still fails formatting:
uv run --extra dev ruff format --check app/main.py app/mcp.py app/mcp_tools.py tests/test_mcp_board.pyreportsapp/mcp_tools.pywould be reformatted.
Validation I ran on this exact head:
uv run --extra dev python -m pytest tests/test_mcp_board.py tests/test_api_mcp.py::test_mcp_tools_list_and_call tests/test_api_mcp.py::test_mcp_get_bounty_rejects_ambiguous_issue_number_selector -q-> 5 passed, 1 existing Starlette/httpx warning.uv run --extra dev python -m pytest tests/test_api_mcp.py tests/test_mcp_tools.py tests/test_mcp_board.py -q-> 3 failed, 112 passed, 1 existing Starlette/httpx warning. The failed tests are the three selector regressions listed above.uv run --extra dev ruff check app/main.py app/mcp.py app/mcp_tools.py tests/test_mcp_board.py-> passed.uv run --extra dev ruff format --check app/main.py app/mcp.py app/mcp_tools.py tests/test_mcp_board.py-> would reformatapp/mcp_tools.py.uv run --extra dev mypy app/mcp.py app/mcp_tools.py-> success.uv run --extra dev python scripts/docs_smoke.py-> docs smoke ok.git diff --check origin/main...HEAD-> clean.git merge-tree --write-tree origin/main HEAD-> clean tree24f2149720bac7b8be23245e2d131b861dafa391.
Suggested direction: keep or restore the shared bounty selector path for get_bounty and list_bounty_attempts, preserve the mixed-selector rejection, then rerun the MCP test files and format check.
Scope note: this review used only public PR code, public GitHub check state, and local tests. I did not use private data, credentials, wallet material, payout execution, treasury mutation, ledger mutation, bridge/exchange/cash-out behavior, or MRWK price behavior.
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: c3a017a6-90c1-4633-bd76-1bbfec425825
📒 Files selected for processing (2)
app/mcp_tools.pytests/test_api_mcp.py
| def find_bounty_from_args(session: Session, *, id_arg_name: str | None = None) -> Bounty | None: | ||
| has_id = ( | ||
| id_arg_name is not None and id_arg_name in args and args.get(id_arg_name) is not None | ||
| ) | ||
| has_issue_number = "issue_number" in args and args.get("issue_number") is not None | ||
| repo_selector = optional_repo_selector_arg() | ||
| if has_internal_id and has_issue_number: | ||
| raise ValueError(f"use {internal_id_field} or issue_number, not both") | ||
|
|
||
| if has_id and has_issue_number: | ||
| raise ValueError(f"use {id_arg_name} or issue_number, not both") | ||
| if repo_selector is not None and not has_issue_number: | ||
| raise ValueError("repo can only be used with issue_number") | ||
| if has_internal_id: | ||
| return session.get(Bounty, positive_int_arg(internal_id_field)) | ||
|
|
||
| if has_id: | ||
| # id_arg_name is not None here | ||
| return session.get(Bounty, positive_int_arg(id_arg_name)) | ||
|
|
||
| if has_issue_number: | ||
| return bounty_by_issue_number(repo_selector) | ||
| raise ValueError(f"{internal_id_field} or issue_number is required") | ||
| issue_query = select(Bounty).where( | ||
| Bounty.issue_number == positive_int_arg("issue_number") | ||
| ) | ||
| if repo_selector is not None: | ||
| issue_query = issue_query.where(func.lower(Bounty.repo) == repo_selector) | ||
| bounties = session.scalars(issue_query.order_by(Bounty.id.desc()).limit(2)).all() | ||
| if not bounties: | ||
| return None | ||
| if len(bounties) > 1: | ||
| raise ValueError("issue_number matches multiple bounties") | ||
| return bounties[0] | ||
|
|
||
| return None |
There was a problem hiding this comment.
Add regression tests for the new selector rules.
find_bounty_from_args() now changes lookup and validation behavior for get_bounty, list_bounty_attempts, and submit_work_proof, but the added coverage shown here only exercises resources/read. Please pin down at least the repo-without-issue_number, mixed id/issue_number, and ambiguous issue_number cases so these tool paths do not drift silently. As per coding guidelines, "Add or update tests for changed behavior".
Also applies to: 209-217, 287-298
Source: Coding guidelines
6 participants
This PR implements the MCP (Model Context Protocol) resource for public bounty board data, as requested in issue #791. It exposes the bounty board data via the mcp://bounties/active resource.
Summary by CodeRabbit