Extract MCP tool dispatch from app.main#356
Conversation
|
Warning Review limit reached
More reviews will be available in 3 minutes and 33 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR extracts MCP tool dispatch into a dedicated app/mcp_tools.py module so MCP tools can be invoked without the FastAPI route wiring, updates the /mcp route to use the extracted dispatcher, and adds tests validating tool behavior against a SQLite DB.
Changes:
- Added
app/mcp_tools.pywithcall_mcp_tool()and supporting validation/serialization helpers. - Updated
app/main.pyto usecall_mcp_toolfor/mcpand hardened_safe_next_pathby validating decoded values. - Added pytest coverage for listing bounties and rejecting invalid ledger arguments without an HTTP layer.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tests/test_mcp_tools.py | Adds tests invoking MCP tools directly against SQLite schema/data. |
| app/mcp_tools.py | Introduces extracted MCP tool dispatcher and argument normalization utilities. |
| app/main.py | Wires /mcp to the extracted dispatcher and strengthens redirect “next” path validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| proof = session.get(Proof, _proof_hash_from_arg(str_arg("hash"))) | ||
| if proof is None: | ||
| return "proof not found" | ||
| public_payload = json.loads(proof.public_json) |
| "ledger_sequence": proof.ledger_sequence, | ||
| "bounty_id": proof.bounty_id, | ||
| "submission_id": proof.submission_id, | ||
| "created_at": proof.created_at.isoformat(), |
| bounty_id = lower.removeprefix(reserve_prefix) | ||
| try: | ||
| normalized_bounty_id = int(bounty_id) if bounty_id.isdigit() else 0 | ||
| except ValueError as exc: | ||
| raise ValueError("reserve bounty id is too large") from exc | ||
| if normalized_bounty_id <= 0: | ||
| raise ValueError("reserve bounty id must be positive") | ||
| if normalized_bounty_id > SQLITE_INTEGER_MAX: | ||
| raise ValueError("reserve bounty id is too large") |
| elif isinstance(value, str): | ||
| clean = value.strip() | ||
| if clean and clean.lstrip("+-").isdigit(): | ||
| try: | ||
| parsed = int(clean) | ||
| except ValueError as exc: | ||
| raise ValueError(f"{field} must be an integer") from exc |
| def test_call_mcp_tool_rejects_invalid_arguments_without_http_layer(sqlite_url: str) -> None: | ||
| create_schema(sqlite_url) | ||
| with session_scope(sqlite_url) as session: | ||
| ensure_genesis(session) | ||
|
|
||
| with pytest.raises(ValueError, match="sequence must be positive"): | ||
| call_mcp_tool(sqlite_url, "get_ledger_entry", {"sequence": 0}) |
tolga-tom-nook
left a comment
tolga-tom-nook
left a comment
There was a problem hiding this comment.
Requested change: current head d164d08 fails the repository formatting gate.
What I checked:
- Inspected
app/mcp_tools.py,app/main.py, andtests/test_mcp_tools.pyagainstorigin/main. - Verified the MCP tool dispatch extraction is mechanically wired through
/mcpviahandle_mcp_request(..., call_mcp_tool)and that the extracted helper preserves the existing session/argument validation/tool dispatch behavior. - Ran
./.venv/bin/python -m pytest tests/test_mcp_tools.py tests/test_api_mcp.py -q— passed, 78 tests. - Ran full
./.venv/bin/python -m pytest -q— passed, 337 tests. - Ran
./.venv/bin/python -m ruff check app/main.py app/mcp_tools.py tests/test_mcp_tools.py tests/test_api_mcp.py— passed. - Ran
./.venv/bin/python -m mypy app— passed. - Ran
./.venv/bin/python scripts/docs_smoke.py— passed. - Ran
git diff --check origin/main...HEAD— passed.
Blocker:
./.venv/bin/python -m ruff format --check .fails withWould reformat: app/mcp_tools.py.- The diff from
./.venv/bin/python -m ruff format --diff app/mcp_tools.pyis only the_bounty_attempt_to_dict(...)signature collapsing to one line, so this should be a tiny formatting-only fix.
After formatting app/mcp_tools.py and re-running the format gate, I do not see functional blockers in the MCP dispatch extraction.
ayskobtw-lil
left a comment
ayskobtw-lil
left a comment
There was a problem hiding this comment.
Follow-up review after the formatting fix in 4462654.
Evidence checked:
- Rechecked current head
44626549aa7b5c6f0364296d05fa42623d990452after the earlier review blocker thatapp/mcp_tools.pyfailed the formatter gate. - Inspected
app/mcp_tools.py,app/main.py, andtests/test_mcp_tools.py; verified/mcpnow delegates throughhandle_mcp_request(..., call_mcp_tool)while the extracted dispatcher preserves directlist_bounties,get_ledger_entry, and existing MCP route coverage. - Confirmed the decoded OAuth next-path guard still covers the wallet login regression included in the focused run.
Validation:
python -m ruff format --check app/mcp_tools.py app/main.py tests/test_mcp_tools.py-> 3 files already formatted.python -m pytest tests/test_mcp_tools.py tests/test_api_mcp.py tests/test_wallet_api.py::test_github_login_stores_safe_default_for_backslash_next -q-> 79 passed.python -m ruff check app/main.py app/mcp_tools.py tests/test_mcp_tools.py tests/test_api_mcp.py tests/test_wallet_api.py-> passed.python -m mypy app/mcp_tools.py app/main.py-> success.git diff --check origin/main...HEAD-> clean.- GitHub reports the hosted Quality/readiness/docs/image check green and the PR mergeable.
The earlier formatting blocker is resolved; I do not see remaining blockers from this follow-up pass.
|
Maintainer note after #320 filled: this PR still references closed bounty #320. If you want it considered for the current code-health round, rebase against main, update the bounty reference to #377, and keep the scope distinct from already accepted #328, #337, #364, #375, and #376. This is not accepted or paid as-is. |
|
Closing this stale bounty submission. It still references a closed/filled bounty round and is dirty or unstable against current main after the route/test extraction batches. A future attempt needs a fresh PR against current main, a currently open bounty reference, and scope distinct from work already accepted. |
Refs #320
Summary
Complexity reduced
Tests