Skip to content

452Remove-Integration-Type#678

Merged
crivetimihai merged 3 commits intoIBM:mainfrom
Nayana-R-Gowda:452Remove-Integration-Type
Aug 9, 2025
Merged

452Remove-Integration-Type#678
crivetimihai merged 3 commits intoIBM:mainfrom
Nayana-R-Gowda:452Remove-Integration-Type

Conversation

@Nayana-R-Gowda
Copy link
Copy Markdown
Collaborator

@Nayana-R-Gowda Nayana-R-Gowda commented Aug 7, 2025 •
edited by crivetimihai
Loading

📌 Summary

What problem does this PR fix and why?
integrationType should only support REST, not MCP closes #452

💡 Fix Description

How did you solve it? Key design points.
The integrationType field in the admin UI (and API) currently allows "MCP", but this is not desired. The issue requests removing "MCP" from all input forms (modals)—keeping only "REST". There’s mention that support for "MCP" servers may eventually be added under a separate "Gateways" section.

🧪 Verification

Check Command Status
Lint suite make lint pass
Unit tests make test pass

📐 MCP Compliance (if relevant)

  • Matches current MCP spec
  • No breaking change to MCP clients

✅ Checklist

  • Code formatted (make black isort pre-commit)
  • No secrets/credentials committed

@Nayana-R-Gowda Nayana-R-Gowda changed the title @pytest.mark.skip("Test temporarily skipped – investigation in progress") 452Remove-Integration-Type Aug 7, 2025
@crivetimihai
Copy link
Copy Markdown
Member

crivetimihai commented Aug 8, 2025

PR Review: Remove MCP Integration Type (#678)

Summary

This PR removes "MCP" as an integration type option from the admin UI and API, keeping only "REST" as the supported integration type. This aligns with the requirement that MCP servers are supported under a separate "Gateways" section.

Changes Reviewed

1. Schema Changes (mcpgateway/schemas.py)

  • Line 311: Changed integration_type field from Literal["MCP", "REST"] to Literal["REST"] with default value "REST"
  • Lines 463-481: Updated validate_request_type to handle REST-only validation
  • Lines 649-660: Updated ToolUpdate class to only allow REST request types

Issues Found:

  • ⚠️ Inconsistent validation logic: The validate_request_type method still has MCP validation code (lines 475-478) even though MCP is no longer a valid option
  • The error messages still reference MCP which could be confusing

2. Admin API Changes (mcpgateway/admin.py)

  • Lines 1741-1750: Added logic to default request_type based on integration_type
  • Line 1972: Changed default integration type to "REST" in edit tool
  • Updated all doctest examples to use REST instead of MCP

Issues Found:

  • ⚠️ Dead code: Lines 1745-1747 check for MCP integration type but MCP is no longer a valid option
  • The defaulting logic for request_type when integration_type == "MCP" will never execute

3. Template Changes (mcpgateway/templates/admin.html)

  • Line 894: Removed <option value="MCP">MCP</option> from add tool modal
  • Line 2324: Removed <option value="MCP">MCP</option> from edit tool modal

Review: ✅ Clean removal of MCP options from UI

4. Test Updates

  • tests/security/test_input_validation.py:
    • Commented out MCP validation tests (lines 498-503)
    • Updated default integration type assertion to "REST"
  • tests/unit/mcpgateway/services/test_tool_service.py:
    • Updated all test cases to use REST methods instead of MCP
  • tests/unit/mcpgateway/test_admin.py:
    • Updated test cases to use REST integration type
  • docs/docs/testing/basic.md:
    • Updated example to use REST instead of MCP

Issues Found:

  • ⚠️ Commented code: Instead of commenting out MCP tests, they should be removed entirely

Test Results

⚠️ Tests cannot run due to database migration issue:

  • Error: alembic.util.exc.CommandError: Can't locate revision identified by 'cc7b95fec5d9'
  • This appears to be an environment issue after rebasing from main, not related to the PR changes

Recommendations

Critical Issues to Fix:

  1. Remove dead MCP code in mcpgateway/schemas.py:

    • Remove lines 475-478 that check for MCP integration type
    • Simplify the validation to only handle REST methods

  2. Remove dead MCP code in mcpgateway/admin.py:

    • Remove lines 1745-1747 that handle MCP integration type
    • Simplify the defaulting logic

  3. Clean up commented tests:

    • Remove commented MCP test code in test_input_validation.py instead of leaving it commented

Minor Improvements:

  1. Update error messages: Remove references to MCP in error messages since it's no longer supported

  2. Consider adding a database migration: If there are existing tools with integration_type="MCP" in the database, a migration might be needed to update them to REST

Overall Assessment

The PR successfully removes MCP as an integration type option from the UI and API. However, there's some dead code left behind that should be cleaned up. The changes are mostly correct but need some refinement to remove references to MCP completely.

NAYANAR0502 and others added 3 commits August 9, 2025 15:31
@NAYANAR0502 @crivetimihai
Remove Integration Type: MCP
6a1b504 
Signed-off-by: NAYANAR <nayana.r5@ibm.com>
@NAYANAR0502 @crivetimihai
testcase fixed
c56fc73 
Signed-off-by: NAYANAR <nayana.r5@ibm.com>
@crivetimihai
Fix validation logic and tests after MCP integration type removal
6784fe5 
- Update ToolCreate and ToolUpdate validation to only accept REST integration type
- Fix test cases that still referenced MCP integration type
- Update mock return values and assertions in test files
- Ensure all doctests pass with REST-only validation
- Maintain full test suite compatibility

All tests now pass: 1307 passed, 10 skipped
@crivetimihai crivetimihai force-pushed the 452Remove-Integration-Type branch from 5645ef6 to 6784fe5 Compare August 9, 2025 14:53
@crivetimihai crivetimihai merged commit 7334633 into IBM:main Aug 9, 2025
36 checks passed
@crivetimihai crivetimihai mentioned this pull request Aug 10, 2025
Closed
@crivetimihai
Copy link
Copy Markdown
Member

crivetimihai commented Aug 10, 2025

Will be reverting this through a separate PR.

vk-playground pushed a commit to vk-playground/mcp-context-forge that referenced this pull request Sep 14, 2025
@Nayana-R-Gowda @NAYANAR0502 @crivetimihai
452Remove-Integration-Type (IBM#678)
9fb6a80 
* Remove Integration Type: MCP

Signed-off-by: NAYANAR <nayana.r5@ibm.com>

* testcase fixed

Signed-off-by: NAYANAR <nayana.r5@ibm.com>

* Fix validation logic and tests after MCP integration type removal

- Update ToolCreate and ToolUpdate validation to only accept REST integration type
- Fix test cases that still referenced MCP integration type
- Update mock return values and assertions in test files
- Ensure all doctests pass with REST-only validation
- Maintain full test suite compatibility

All tests now pass: 1307 passed, 10 skipped

---------

Signed-off-by: NAYANAR <nayana.r5@ibm.com>
Co-authored-by: NAYANAR <nayana.r5@ibm.com>
Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>
vk-playground pushed a commit to vk-playground/mcp-context-forge that referenced this pull request Sep 14, 2025
@Nayana-R-Gowda @NAYANAR0502 @crivetimihai
452Remove-Integration-Type (IBM#678)
090aa9c 
* Remove Integration Type: MCP

Signed-off-by: NAYANAR <nayana.r5@ibm.com>

* testcase fixed

Signed-off-by: NAYANAR <nayana.r5@ibm.com>

* Fix validation logic and tests after MCP integration type removal

- Update ToolCreate and ToolUpdate validation to only accept REST integration type
- Fix test cases that still referenced MCP integration type
- Update mock return values and assertions in test files
- Ensure all doctests pass with REST-only validation
- Maintain full test suite compatibility

All tests now pass: 1307 passed, 10 skipped

---------

Signed-off-by: NAYANAR <nayana.r5@ibm.com>
Co-authored-by: NAYANAR <nayana.r5@ibm.com>
Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>
vk-playground pushed a commit to vk-playground/mcp-context-forge that referenced this pull request Sep 16, 2025
@Nayana-R-Gowda @NAYANAR0502 @crivetimihai
452Remove-Integration-Type (IBM#678)
79f723c 
* Remove Integration Type: MCP

Signed-off-by: NAYANAR <nayana.r5@ibm.com>

* testcase fixed

Signed-off-by: NAYANAR <nayana.r5@ibm.com>

* Fix validation logic and tests after MCP integration type removal

- Update ToolCreate and ToolUpdate validation to only accept REST integration type
- Fix test cases that still referenced MCP integration type
- Update mock return values and assertions in test files
- Ensure all doctests pass with REST-only validation
- Maintain full test suite compatibility

All tests now pass: 1307 passed, 10 skipped

---------

Signed-off-by: NAYANAR <nayana.r5@ibm.com>
Co-authored-by: NAYANAR <nayana.r5@ibm.com>
Co-authored-by: Mihai Criveti <crivetimihai@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@crivetimihai crivetimihai Awaiting requested review from crivetimihai crivetimihai is a code owner

@kevalmahajan kevalmahajan Awaiting requested review from kevalmahajan kevalmahajan is a code owner

@madhav165 madhav165 Awaiting requested review from madhav165 madhav165 is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG]: integrationType should only support REST

3 participants

@Nayana-R-Gowda @crivetimihai @NAYANAR0502