Extract MCP tool dispatcher from app.main#398
Conversation
📝 WalkthroughWalkthroughExtracts MCP tool dispatch into ChangesMCP Tool Extraction Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🤖 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 `@app/mcp_tools.py`:
- Around line 239-241: submit_work_proof builds issue_query and filters by repo
using Bounty.repo == repo_selector while repo_selector is lowercased; change
this to a case-insensitive comparison to match other code paths by comparing
func.lower(Bounty.repo) to repo_selector (or applying func.lower to both sides)
so the repo filter does not miss bounties when Bounty.repo is not stored
lowercase; update the filter on issue_query (where clause referencing
Bounty.repo and repo_selector) to use func.lower(Bounty.repo) == repo_selector
instead of Bounty.repo == repo_selector.
🪄 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: 2903605b-ad7e-411f-8582-b4569b9bfef3
📒 Files selected for processing (3)
app/main.pyapp/mcp_tools.pytests/test_mcp_tools.py
|
Addressed the CodeRabbit repo-selector note in Change:
Validation after the fix:
|
tolga-tom-nook
left a comment
tolga-tom-nook
left a comment
There was a problem hiding this comment.
No blockers from my review of the MCP tool dispatcher extraction at head beb4010.
Evidence checked:
app/main.pynow importscall_mcp_tooland keeps the MCP request boundary athandle_mcp_request(request, db_url, call_mcp_tool).app/mcp_tools.pypreserves the extracted behavior for bounty listing/search, bounty lookup with awards, attempt listing, balance/wallet/transfer/ledger/proof tools, andsubmit_work_proofguidance including repo selector validation.tests/test_mcp_tools.pyadds direct coverage for the extracted dispatcher and mixed-case stored repo matching.
Validation run locally:
/home/ubuntu/tom-agent/work/mergework/.venv/bin/python -m pytest tests/test_mcp_tools.py tests/test_api_mcp.py -q→ 86 passed/home/ubuntu/tom-agent/work/mergework/.venv/bin/python -m pytest -q→ 382 passed/home/ubuntu/tom-agent/work/mergework/.venv/bin/python -m ruff check app/mcp_tools.py app/main.py tests/test_mcp_tools.py tests/test_api_mcp.py→ passed/home/ubuntu/tom-agent/work/mergework/.venv/bin/python -m ruff format --check app/mcp_tools.py app/main.py tests/test_mcp_tools.py tests/test_api_mcp.py→ passed/home/ubuntu/tom-agent/work/mergework/.venv/bin/python -m mypy app/mcp_tools.py app/main.py→ passedgit diff --check origin/main...HEAD→ passed
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/test_mcp_tools.py`:
- Around line 33-55: The test currently passes even if the repo argument is
ignored because only one bounty exists; create a decoy bounty with the same
issue_number but a different repo and title (using create_bounty) before calling
call_mcp_tool in test_submit_work_proof_repo_selector_matches_stored_repo_case,
then call call_mcp_tool with repo="ramimbo/mergework" and assert the returned
result contains the expected "Code health bounty" and "Bounty `#390`" and does NOT
contain the decoy bounty's title to prove the repo-selector is honored (keep
existing case-variant bounty too to validate case-insensitive matching).
🪄 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: 2e64fcc8-8337-41fd-8465-5c817f5e1bd9
📒 Files selected for processing (2)
app/mcp_tools.pytests/test_mcp_tools.py
| def test_submit_work_proof_repo_selector_matches_stored_repo_case(sqlite_url: str) -> None: | ||
| create_schema(sqlite_url) | ||
| with session_scope(sqlite_url) as session: | ||
| ensure_genesis(session) | ||
| bounty = create_bounty( | ||
| session, | ||
| repo="ramimbo/mergework", | ||
| issue_number=390, | ||
| issue_url="https://github.com/ramimbo/mergework/issues/390", | ||
| title="Code health bounty", | ||
| reward_mrwk="200", | ||
| acceptance="Extract a coherent subsystem from app.main.", | ||
| ) | ||
| bounty.repo = "Ramimbo/MergeWork" | ||
|
|
||
| result = call_mcp_tool( | ||
| sqlite_url, | ||
| "submit_work_proof", | ||
| {"issue_number": 390, "repo": "ramimbo/mergework"}, | ||
| ) | ||
|
|
||
| assert "Code health bounty" in result | ||
| assert "Bounty #390" in result |
There was a problem hiding this comment.
Strengthen this regression test to actually validate repo-selector behavior.
With only one bounty in the database, this test still passes if the repo argument is ignored and lookup uses only issue_number. Add a decoy bounty sharing the same issue number under a different repo, and assert it is not selected.
Proposed test hardening
def test_submit_work_proof_repo_selector_matches_stored_repo_case(sqlite_url: str) -> None:
create_schema(sqlite_url)
with session_scope(sqlite_url) as session:
ensure_genesis(session)
- bounty = create_bounty(
+ bounty = create_bounty(
session,
repo="ramimbo/mergework",
issue_number=390,
issue_url="https://github.com/ramimbo/mergework/issues/390",
title="Code health bounty",
reward_mrwk="200",
acceptance="Extract a coherent subsystem from app.main.",
)
+ create_bounty(
+ session,
+ repo="otherorg/otherrepo",
+ issue_number=390,
+ issue_url="https://github.com/otherorg/otherrepo/issues/390",
+ title="Decoy bounty",
+ reward_mrwk="200",
+ acceptance="Decoy acceptance criteria.",
+ )
bounty.repo = "Ramimbo/MergeWork"
result = call_mcp_tool(
sqlite_url,
"submit_work_proof",
{"issue_number": 390, "repo": "ramimbo/mergework"},
)
assert "Code health bounty" in result
assert "Bounty `#390`" in result
+ assert "Decoy bounty" not in resultAs per coding guidelines, tests/**/*.py: Add or update tests for changed behavior.
🤖 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_mcp_tools.py` around lines 33 - 55, The test currently passes even
if the repo argument is ignored because only one bounty exists; create a decoy
bounty with the same issue_number but a different repo and title (using
create_bounty) before calling call_mcp_tool in
test_submit_work_proof_repo_selector_matches_stored_repo_case, then call
call_mcp_tool with repo="ramimbo/mergework" and assert the returned result
contains the expected "Code health bounty" and "Bounty `#390`" and does NOT
contain the decoy bounty's title to prove the repo-selector is honored (keep
existing case-variant bounty too to validate case-insensitive matching).
rebel117
left a comment
rebel117
left a comment
There was a problem hiding this comment.
Reviewed the MCP tool dispatcher extraction. Overall a clean extraction from app/main.py into app/mcp_tools.py.
One observation: the submit_work_proof handler introduces a behavioral change. The original code in main.py does an exact match:
issue_query = issue_query.where(Bounty.repo == repo_selector)The extracted version uses case-insensitive matching:
issue_query = issue_query.where(func.lower(Bounty.repo) == repo_selector)Since optional_repo_selector_arg() already lowercases the input, the original code would only match repos stored in lowercase. The new code also matches mixed-case stored repos like Ramimbo/MergeWork. This is actually an improvement (the test test_submit_work_proof_repo_selector_matches_stored_repo_case covers this), but it is a behavioral change beyond a pure extraction. Worth noting in the PR description so reviewers/maintainers are aware.
Checked the rest of the extraction — imports, argument validators, and tool dispatch paths are preserved correctly. The 3 tests cover the main paths (list, case-insensitive repo search, validation errors). The remaining tool handlers (get_bounty, list_bounty_attempts, get_balance, register_wallet, get_wallet, submit_wallet_transfer, get_ledger_entry, get_proof) are untested in this PR but the extraction itself is faithful to the original.
3 participants
Summary
app/main.pyintoapp/mcp_tools.py./mcproute wiring inapp/main.py, passing the extracted dispatcher intohandle_mcp_request.Evidence
app/main.pystill owned the full MCP tool dispatch/argument-validation block, making app wiring harder to review separately from MCP tool behavior. This creates a focused MCP boundary without changing public MCP responses.Test Evidence
ruff format --check .-> 67 files already formattedruff check .-> All checks passedmypy app-> Success: no issues found in 24 source filespytest-> 381 passedpython scripts/docs_smoke.py-> docs smoke okgit diff --check-> cleanMRWK
Related bounty or issue (
Bounty #NorRefs #Nfor multi-award bounties): Bounty #390Summary by CodeRabbit
New Features
Refactor
Tests