Scope MCP work-proof issue lookup by repo#351
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR extends the submit_work_proof MCP tool to accept an optional ChangesRepo selector feature for issue number disambiguation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
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 extends the MCP submit_work_proof tool so that selecting a bounty by issue_number can be disambiguated by providing a repo selector, and adds tests for the new behavior and validation.
Changes:
- Add optional
repoargument parsing/validation for MCPsubmit_work_proof. - Filter bounty lookup by
repowhen selecting byissue_number. - Add test coverage for repo-scoped selection and new invalid selector cases.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/test_api_mcp.py | Adds coverage for repo-scoped bounty selection and repo argument validation. |
| app/mcp.py | Updates MCP tool description to mention repo scoping and JSON/text output. |
| app/main.py | Implements repo selector parsing and applies it to the issue_number lookup query. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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) |
| create_schema(sqlite_url) | ||
| with session_scope(sqlite_url) as session: | ||
| ensure_genesis(session) | ||
| create_bounty( | ||
| session, | ||
| repo="ramimbo/mergework", | ||
| issue_number=284, | ||
| issue_url="https://github.com/ramimbo/mergework/issues/284", | ||
| title="First bounty", | ||
| reward_mrwk="100", | ||
| acceptance="First acceptance.", | ||
| ) | ||
| target = create_bounty( | ||
| session, | ||
| repo="example/mergework", | ||
| issue_number=284, | ||
| issue_url="https://github.com/example/mergework/issues/284", | ||
| title="Second bounty", | ||
| reward_mrwk="250", | ||
| acceptance="Second acceptance.", | ||
| ) | ||
| target_id = target.id | ||
|
|
||
| client = TestClient(create_app(database_url=sqlite_url, webhook_secret="secret")) |
| "Return submission instructions, optionally for a bounty_id or issue_number " | ||
| "and repo, with text or json format" |
TateLyman
left a comment
TateLyman
left a comment
There was a problem hiding this comment.
Reviewed the repo-scoped submit_work_proof lookup for Bounty #315.\n\nWhat I checked:\n- app/main.py: repo is accepted only with issue_number, validated as a non-empty string through the existing clean-string helper, capped at 200 chars, lowercased, and applied as func.lower(Bounty.repo) == repo_selector before the existing two-row ambiguity check.\n- app/mcp.py: tool description now advertises the repo selector alongside text/json output.\n- tests/test_api_mcp.py: added a duplicate-issue fixture proving repo: Example/MergeWork selects the intended example/mergework bounty, plus invalid selector coverage for repo without issue_number, repo with bounty_id, non-string repo, and oversized repo.\n\nValidation run locally on current head d16094c:\n- uv run --extra dev python -m pytest tests/test_api_mcp.py::test_mcp_submit_work_proof_scopes_issue_number_by_repo tests/test_api_mcp.py::test_mcp_submit_work_proof_rejects_invalid_bounty_selectors tests/test_api_mcp.py::test_mcp_submit_work_proof_rejects_ambiguous_issue_number -q -> 13 passed.\n- uv run --extra dev python -m pytest tests/test_api_mcp.py -q -> 81 passed.\n- uv run --extra dev ruff check app/main.py app/mcp.py tests/test_api_mcp.py -> passed.\n- uv run --extra dev ruff format --check app/main.py app/mcp.py tests/test_api_mcp.py -> passed.\n- uv run --extra dev python -m mypy app/main.py app/mcp.py -> passed.\n- uv run --extra dev python scripts/docs_smoke.py -> docs smoke ok.\n- git diff --check origin/main...HEAD -> clean.\n\nNo blocker found on this head. Caveat for merge sequencing: this branch touches app/mcp.py near the same tool metadata recently changed by PR #345, so it should be rechecked if #345 lands first.
TUPM96
force-pushed
the
codex/mcp-repo-selector-315
branch
from
d16094c to
77be52e
Compare
|
Updated after review:
Revalidated:
|
AugmentSecurity
left a comment
AugmentSecurity
left a comment
There was a problem hiding this comment.
Reviewed PR #351 on head d16094c.
No blockers found. Evidence checked:
- Inspected
app/main.pyand confirmedrepois accepted only withissue_number, normalized through the existing clean-string path plus lowercase conversion, capped at 200 chars, and applied asfunc.lower(Bounty.repo) == repo_selectorbefore the existing two-row ambiguity check. - Confirmed the
bounty_idselector path remains separate, sorepocannot alter direct bounty-id lookup. - Inspected
app/mcp.py; this branch exposes MCP tool descriptions only, so there is noinputSchemamismatch to update here. - Inspected
tests/test_api_mcp.pyand confirmed coverage for duplicate issue numbers across repos, case-insensitive repo selection, JSON structured output identity, repo-only rejection, repo-with-bounty-id rejection, non-string repo rejection, and oversized repo rejection.
Validation:
./.venv/bin/python -m pytest tests/test_api_mcp.py::test_mcp_submit_work_proof_scopes_issue_number_by_repo tests/test_api_mcp.py::test_mcp_submit_work_proof_rejects_invalid_bounty_selectors -q-> 12 passed../.venv/bin/python -m pytest tests/test_api_mcp.py -q-> 81 passed../.venv/bin/python -m ruff check app/main.py app/mcp.py tests/test_api_mcp.py-> passed../.venv/bin/python -m ruff format --check app/main.py app/mcp.py tests/test_api_mcp.py-> passed../.venv/bin/python -m mypy app/main.py app/mcp.py-> passed.git diff --check origin/main...HEAD-> clean.
AugmentSecurity
left a comment
AugmentSecurity
left a comment
There was a problem hiding this comment.
Correction after the head moved while my previous review was being submitted: current head 77be52e has blockers.
Requested changes:
tests/test_api_mcp.py::test_mcp_tools_list_and_callnow fails because the updatedsubmit_work_proofdescription no longer contains the existing expected phrasebounty_id or issue_number. Hosted CI is failing for the same reason.- The current lookup uses
Bounty.repo == repo_selectorwhilerepo_selectoris lowercased. That no longer provides the case-insensitive repository matching described in the PR body and tool behavior; it only works when stored repo names are already lowercase. Usefunc.lower(Bounty.repo) == repo_selectoror add equivalent normalization and a mixed-case stored-repo regression.
Validation on current head:
./.venv/bin/python -m pytest tests/test_api_mcp.py::test_mcp_tools_list_and_call -q-> fails on the description assertion above../.venv/bin/python -m pytest tests/test_api_mcp.py::test_mcp_submit_work_proof_scopes_issue_number_by_repo tests/test_api_mcp.py::test_mcp_submit_work_proof_rejects_invalid_bounty_selectors -q-> 12 passed.git diff --check origin/main...HEAD-> clean.
TateLyman
left a comment
TateLyman
left a comment
There was a problem hiding this comment.
Correction: my earlier no-blockers review was for the previous head d16094c. Current head 77be52e changed while review/claim activity was happening and now has blockers.\n\nRequested changes:\n- tests/test_api_mcp.py::test_mcp_tools_list_and_call fails because the updated tool description no longer contains the existing expected phrase bounty_id or issue_number. Hosted CI is failing on this head as well. Keep the old phrase or update the assertion intentionally with equivalent coverage.\n- The repo lookup now compares Bounty.repo == repo_selector while repo_selector is lowercased. That no longer provides the case-insensitive repository matching described in the PR body unless stored repo names are always already normalized. The previous func.lower(Bounty.repo) == repo_selector version was safer, or add an explicit mixed-case stored-repo regression if relying on normalized storage.\n\nValidation on current head 77be52e:\n- uv run --extra dev python -m pytest tests/test_api_mcp.py::test_mcp_tools_list_and_call -q -> fails on the description assertion.\n- uv run --extra dev python -m pytest tests/test_api_mcp.py::test_mcp_submit_work_proof_scopes_issue_number_by_repo tests/test_api_mcp.py::test_mcp_submit_work_proof_rejects_invalid_bounty_selectors -q -> 12 passed.\n\nThe repo-scoped selection idea is still useful, but this head needs the CI failure fixed before merge.
TUPM96
force-pushed
the
codex/mcp-repo-selector-315
branch
from
77be52e to
0473eba
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/main.py`:
- Around line 1782-1787: The repo filter on issue_query uses a case-sensitive
equality (Bounty.repo == repo_selector) while other lookups like
find_bounty_by_issue use func.lower(Bounty.repo) for case-insensitive matching;
update the where clause that references repo_selector to compare
lower(Bounty.repo) to a lower-cased repo_selector (e.g. func.lower(Bounty.repo)
== repo_selector.lower()) and ensure func is imported from sqlalchemy so repo
lookups are consistently case-insensitive across functions (reference Bounty,
issue_query, repo_selector, and find_bounty_by_issue).
In `@app/mcp.py`:
- Around line 41-44: The test assertion still expects the old description text
"bounty_id or issue_number"; update the assertion in the failing test to expect
the new description string used in app.mcp (i.e. "Return submission instructions
for bounty_id, or issue_number optionally scoped by repo, with text or json
format") so the test matches the updated tool description.
🪄 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: 5a6a02bc-423a-49b6-a55c-81d214fa0c16
📒 Files selected for processing (3)
app/main.pyapp/mcp.pytests/test_api_mcp.py
TateLyman
left a comment
TateLyman
left a comment
There was a problem hiding this comment.
Rechecked current head 0473eba after the description fix. The hosted test failure from my previous review is fixed: test_mcp_tools_list_and_call now passes, and the description still preserves the discoverable bounty_id or issue_number wording while adding repo scoping.\n\nOn the repo comparison: create_bounty() normalizes stored repo names through _normalize_repo_name(repo).lower(), and optional_repo_selector_arg() lowercases the selector, so the current equality check is consistent with stored data. A mixed-case stored-repo regression would be extra defense, but I no longer consider this a blocker for this PR.\n\nValidation on current head:\n- uv run --extra dev python -m pytest tests/test_api_mcp.py::test_mcp_tools_list_and_call tests/test_api_mcp.py::test_mcp_submit_work_proof_scopes_issue_number_by_repo tests/test_api_mcp.py::test_mcp_submit_work_proof_rejects_invalid_bounty_selectors -q -> 13 passed.\n- uv run --extra dev python -m mypy app/main.py app/mcp.py -> passed.\n- uv run --extra dev ruff check app/main.py app/mcp.py tests/test_api_mcp.py -> passed.\n\nNo blocker remains from my side.
|
Final recheck after the description compatibility tweak:
|
Refs #315
Summary
reposelector to MCPsubmit_work_proofwhen selecting byissue_number.repounlessissue_numberis present, and validate non-string or oversized repo selectors.Evidence
uv run --extra dev python -m pytest tests/test_api_mcp.py::test_mcp_submit_work_proof_scopes_issue_number_by_repo tests/test_api_mcp.py::test_mcp_submit_work_proof_rejects_invalid_bounty_selectors -q-> 12 passed.uv run --extra dev ruff check app/main.py app/mcp.py tests/test_api_mcp.py-> passed.uv run --extra dev ruff format --check app/main.py app/mcp.py tests/test_api_mcp.py-> 3 files already formatted.uv run --extra dev python -m mypy app/main.py app/mcp.py-> success.git diff --check origin/main..HEAD-> clean.No private keys, seed material, secrets, deployment credentials, private vulnerability details, payout credentials, or MRWK price claims are included.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests