Extract MCP work proof guidance helpers#354
Conversation
|
Warning Review limit reached
More reviews will be available in 5 minutes and 15 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 (1)
📝 WalkthroughWalkthroughThis PR refactors MCP work-proof guidance generation by extracting helper functions into ChangesMCP Guidance Refactoring & Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
liangtovi-debug
left a comment
liangtovi-debug
left a comment
There was a problem hiding this comment.
Requested changes: the ledger typing cleanup breaks the required mypy check.
The PR removes the CursorResult[Any] casts around conditional update(...) calls in app/ledger/service.py, but pay_bounty() and close_bounty() still read claimed.rowcount. With SQLAlchemy typing, session.execute(update(...)) is inferred as Result[Any], so .rowcount is not available to mypy.
Local repro on PR head 682e960:
uv run --extra dev python -m mypy app/ledger/service.py app/main.py app/mcp.py-> fails withResult[Any] has no attribute rowcountatapp/ledger/service.py:670andapp/ledger/service.py:741.uv run --extra dev python -m pytest tests/test_mcp_guidance.py tests/test_api_mcp.py -q-> 78 passed.uv run --extra dev ruff check app/main.py app/mcp.py app/ledger/service.py tests/test_mcp_guidance.py-> passed.uv run --extra dev ruff format --check app/main.py app/mcp.py app/ledger/service.py tests/test_mcp_guidance.py-> passed.git diff --check origin/main...HEAD-> clean.
Please either restore the CursorResult[Any] cast/import around those update results or otherwise type the result as a cursor result before checking rowcount. Since that change is unrelated to the MCP guidance extraction, the lowest-risk fix is probably to drop the ledger cleanup from this PR.
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 “work proof guidance” helpers into app.mcp, adds tests for their output shape, tightens redirect next_path sanitization by checking decoded values, and simplifies SQLAlchemy DML execution typing in ledger service code.
Changes:
- Move
work_proof_guidance*helpers out ofapp.mainintoapp.mcp - Add unit tests covering guidance text/JSON structure
- Harden
_safe_next_pathby validating decoded values; simplifysession.execute(update(...))usage
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
tests/test_mcp_guidance.py |
Adds tests asserting guidance helpers produce expected text and JSON fields. |
app/mcp.py |
Introduces reusable guidance helper functions (text + JSON variants). |
app/main.py |
Uses imported guidance helpers and improves _safe_next_path by validating decoded paths. |
app/ledger/service.py |
Removes explicit casting around session.execute(update(...)) results. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _safe_next_path(next_path: str | None) -> str: | ||
| decoded_next_path = unquote(next_path) if next_path else "" | ||
| if ( | ||
| not next_path | ||
| or not next_path.startswith("/") | ||
| or next_path.startswith("//") | ||
| or len(next_path) > 2048 | ||
| or "\\" in next_path | ||
| or decoded_next_path.startswith("//") | ||
| or "\\" in decoded_next_path | ||
| or any(ord(char) < 32 or 127 <= ord(char) < 160 for char in next_path) | ||
| or any(ord(char) < 32 or 127 <= ord(char) < 160 for char in decoded_next_path) | ||
| ): | ||
| return "/me" | ||
| return next_path |
| def generic_work_proof_guidance_json() -> dict[str, Any]: | ||
| return { | ||
| "bounty_id": None, | ||
| "issue_number": None, | ||
| "status": "generic_guidance", | ||
| "availability": "unknown_without_bounty", | ||
| "can_submit": None, | ||
| "availability_warnings": [], | ||
| "awards_remaining": None, | ||
| "reward_mrwk": None, | ||
| "repository": None, | ||
| "issue_url": None, | ||
| "acceptance": None, | ||
| "submission_format": ( | ||
| "Open a focused PR or issue, reference the MRWK bounty, include test " | ||
| "evidence, and wait for a maintainer to apply mrwk:accepted." | ||
| ), | ||
| "safety_rules": [ | ||
| "Do not include private keys, seed material, secrets, deployment " | ||
| "credentials, private vulnerability details, or price claims." | ||
| ], | ||
| } |
| "submission_format": ( | ||
| "Open a focused PR or issue that links this bounty, include specific " | ||
| "test or behavior evidence, then comment /claim with the PR or " | ||
| "evidence URL and verification summary." | ||
| ), | ||
| "safety_rules": [ | ||
| "Do not include private keys, seed material, secrets, deployment " | ||
| "credentials, private vulnerability details, or price claims." | ||
| ], |
| with session_scope(sqlite_url) as session: | ||
| ensure_genesis(session) | ||
| bounty = create_bounty( |
newmattock
left a comment
newmattock
left a comment
There was a problem hiding this comment.
Follow-up review on current head 015c68b: the earlier mypy blocker from the removed CursorResult[Any] casts is resolved. The PR now keeps the rowcount paths type-checkable with explicit Any annotations, and the MCP guidance extraction remains focused in app.mcp with direct helper coverage.
Evidence checked:
- Inspected
app/mcp.py,app/main.py,app/ledger/service.py, andtests/test_mcp_guidance.py. - Verified
work_proof_guidance,work_proof_guidance_json, andgeneric_work_proof_guidance_jsonmoved out ofapp.mainwithout changing the/mcp submit_work_proofbehavior covered by endpoint tests. - Rechecked the exact earlier blocker:
pay_bounty()andclose_bounty()still readclaimed.rowcount, and current typing no longer fails on those paths. - Confirmed the OAuth encoded-next hardening remains present on current head.
Validation:
uv run --extra dev python -m mypy app/ledger/service.py app/main.py app/mcp.py-> success.uv run --extra dev python -m pytest tests/test_mcp_guidance.py tests/test_api_mcp.py tests/test_wallet_api.py::test_github_login_stores_safe_default_for_backslash_next -q-> 79 passed.uv run --extra dev python -m ruff check app/main.py app/mcp.py app/ledger/service.py tests/test_mcp_guidance.py-> passed.uv run --extra dev python -m ruff format --check app/main.py app/mcp.py app/ledger/service.py tests/test_mcp_guidance.py-> 4 files already formatted.
|
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
submit_work_proofguidance shaping out ofapp/main.pyintoapp/mcp.pyso MCP-specific response text and structured JSON live beside the MCP transport helpers.tests/test_api_mcp.py.CursorResultcasts required forrowcounttype-checking and keeping the existing OAuth redirect hardening intact.Complexity reduced
app/main.pyno longer carries the work-proof guidance message builders. Future MCP guidance changes can be reviewed in the MCP module with focused helper tests instead of inside the route/API monolith.Tests
python -m pytest-> 337 passedpython -m pytest tests/test_mcp_guidance.py tests/test_api_mcp.py tests/test_wallet_api.py::test_github_login_stores_safe_default_for_backslash_next-> 79 passedpython -m ruff format --check .-> 49 files already formattedpython -m ruff check .-> All checks passedpython -m mypy app-> Success: no issues found in 14 source files