Skip to content

feat: replace the internal plugin framework with CPEX#3754

Merged
brian-hussey merged 56 commits intomainfrom
feat/cpex
May 6, 2026
Merged

feat: replace the internal plugin framework with CPEX#3754
brian-hussey merged 56 commits intomainfrom
feat/cpex

Conversation

@araujof
Copy link
Copy Markdown
Member

@araujof araujof commented Mar 20, 2026

Summary

Replace the internal plugin framework (mcpgateway/plugins/framework/, mcpgateway/plugins/tools/, plugin_templates/) with the CPEX external package (cpex>=0.1.0rc1).

Closes: #3753

What changed

  • Added cpex as external dependency in pyproject.toml
  • Migrated all imports: from mcpgateway.plugins.frameworkfrom cpex.framework, from mcpgateway.plugins.toolsfrom cpex.tools
  • Moved get_plugin_manager() singleton factory to mcpgateway/plugins/__init__.py (bridges cpex with gateway-specific hook policies)
  • Migrated PluginMode enums: ENFORCESEQUENTIAL, PERMISSIVETRANSFORM, ENFORCE_IGNORE_ERRORSEQUENTIAL + on_error: ignore (see feat: replace the internal plugin framework with CPEX #3754 (comment))
  • Adapted to cpex API changes:
    • PluginViolation.http_status_code / .http_headersviolation.details["http_status_code"] / details["http_headers"]
    • PromptPosthookPayload.name.prompt_id; ToolPreInvokePayload.arguments.args
  • Deleted internal framework: mcpgateway/plugins/framework/, mcpgateway/plugins/tools/, plugin_templates/ directory, tests/unit/mcpgateway/plugins/framework/
  • Created acceptance tests (tests/acceptance/plugins/test_cpex_contract.py) verifying cpex API surface, behavioral contracts, serialization, and settings compatibility
  • Updated YAML configs across plugins and test fixtures
  • Updated documentation under docs/docs/using/plugins/, plugins/README.md, plugins/AGENTS.md, llms/plugins-llms.md

Test results

  • Unit tests pass
  • Acceptance tests pass
  • Performance profiling shows no regression
  • flake8, bandit, pylint all clean

araujof added 2 commits March 20, 2026 01:57
Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
@araujof araujof added enhancement New feature or request design Architecture and Design plugins MUST P1: Non-negotiable, critical requirements without which the product is non-functional or unsafe labels Mar 20, 2026
@araujof araujof requested a review from terylt March 20, 2026 05:22
@crivetimihai crivetimihai added this to the Release 1.0.0 milestone Mar 20, 2026
araujof added 3 commits April 10, 2026 01:02
Merge upstream/main into feat/cpex (cpex plugin framework migration).

Key resolutions:
- All mcpgateway.plugins.framework imports migrated to cpex.framework
- Plugin singleton functions (get_plugin_manager, enable_plugins, etc.)
  moved to mcpgateway.plugins gateway module
- PluginMode.ENFORCE/PERMISSIVE mapped to SEQUENTIAL/AUDIT (cpex modes)
- PluginViolation.http_status_code moved into details dict (cpex drops
  unknown kwargs)
- Rate limiter http_headers moved to violation.details and result.metadata
- cpex dependency updated to feat/cf_sync branch
- Legacy mode mapping added to gateway_plugin_manager for DB bindings

Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Update remaining mcpgateway.plugins.framework imports in integration
tests to use cpex.framework, and map legacy PluginMode values.

Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
@araujof araujof marked this pull request as ready for review April 10, 2026 05:19
@araujof
Copy link
Copy Markdown
Member Author

araujof commented Apr 10, 2026

Ready for review. Before merging, need to update CPEX to a tagged release. Currently targeting feature branch feat/cf_syncin CPEX repo, which contains backports of in-tree changes to the plugin framework made in the CF repo.

Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
araujof added 3 commits April 10, 2026 01:39
Reorder first-party imports so cpex.framework and mcpgateway imports
are not interleaved, fixing ungrouped-imports warnings.

Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
@araujof araujof requested a review from brian-hussey as a code owner April 13, 2026 20:55
Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
@araujof
Copy link
Copy Markdown
Member Author

araujof commented Apr 14, 2026

@jonpspri @cafalchio we need to branch and update the cpex-* plugin packages with cpex imports for the corresponding tests to pass.

  1. The published cpex-* plugin packages (0.1.0–0.2.0) import from mcpgateway.plugins.framework, which this branch removed in favor of cpex.framework. These packages need new releases that import from cpex.framework.
  2. Until those packages are published, any test that imports them will fail at collection time.

@cafalchio feat/cpex is tracking a branch of cpex where I back-ported the in-tree changes to the plugin framework made after the creation of cpex. See the PR description for the documentation. I tagged it 0.1.0.dev11.

araujof added 3 commits May 5, 2026 21:05
Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
…6906674 to 10.1-1777957922 in Containerfile.lite.

Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
@araujof araujof requested a review from msureshkumar88 May 6, 2026 02:55
@araujof
Copy link
Copy Markdown
Member Author

araujof commented May 6, 2026

@msureshkumar88 thanks for the re-review. All remaining items are addressed.

The missing <0.2 upper bound means a cpex 0.2.0 release with breaking API changes would silently break production

Added. Now "cpex>=0.1.0,<0.2". Worth noting that exclude-newer-package and uv.lock already prevent surprise upgrades in CI/container builds, but the upper bound is good defensive practice for bare pip install deployments.

API clients or existing DB rows with mode="enforce_ignore_error" will fail Pydantic validation

Added as a deprecated enum member:

ENFORCE_IGNORE_ERROR = "enforce_ignore_error"  # Deprecated: use SEQUENTIAL + on_error=ignore

This ensures existing DB rows and API clients serialize/deserialize correctly while guiding users toward the new pattern.

gateway_plugin_manager.py:427-434 uses a sequential for loop while invalidate_all was correctly fixed to use asyncio.gather

Fixed — invalidate_team now uses the same asyncio.gather pattern as invalidate_all, with per-context failure logging and a summary debug line.

When HMAC is configured and an unsigned message arrives, this is invisible in production (debug is filtered)

Promoted to WARNING:

_logger.warning("Plugin invalidation: received unsigned message while HMAC is configured — expected only during rolling deploy")

_sign_message and _verify_and_extract have no dedicated unit tests

Added TestHMACSigningVerification class with 9 tests covering:

  • Sign + verify roundtrip
  • Tampered signature → rejected
  • Tampered payload → rejected
  • Missing payload key in envelope → treated as unsigned
  • Unsigned accepted when no key configured
  • Unsigned accepted with warning when key is set
  • Invalid JSON → returns None
  • _sign_message passthrough when no key
  • Signed envelope extraction when receiver has no key (rolling deploy)

start_plugin_invalidation_listener probe failure at line 539 still logs at WARNING

Keeping as-is for now. The probe failure is a connectivity issue (Redis down), not a security event. Escalating to ERROR would trigger noise in environments where Redis is optional. The exponential backoff + WARNING→ERROR escalation on the listener loop itself already handles the "persistent failure" case.

@araujof
Copy link
Copy Markdown
Member Author

araujof commented May 6, 2026

@brian-hussey @jonpspri This PR is ready for final review and merge.

CI is passing, prior review feedback has been addressed, e2e tests performed by @cafalchio @msureshkumar88 didn't find any regressions, documentation and migration guides are included, and the acceptance test suite validates the cpex contract surface. The massive net deletion demonstrates successful extraction of the framework into a reusable package without functionality loss.

Key changes

The internal framework code that was extracted into cpex is removed, and all plugins, tests, services, and routers are migrated to import from the external package. A gateway-specific bridge layer in mcpgateway/plugins/__init__.py and gateway_plugin_manager.py wires cpex into the application's DI, RBAC, and observability layers.

Area Summary
Dependency Added cpex>=0.1.0,<0.2.0 to pyproject.toml
Import migration from mcpgateway.plugins.frameworkfrom cpex.framework; from mcpgateway.plugins.toolsfrom cpex.tools
Enum migration ENFORCESEQUENTIAL, PERMISSIVETRANSFORM, ENFORCE_IGNORE_ERRORSEQUENTIAL + on_error: ignore
API adaptations PluginViolation.http_status_codeviolation.details["http_status_code"]; PromptPosthookPayload.name.prompt_id; ToolPreInvokePayload.arguments.args
Deleted Internal framework (~35k LOC), plugin cookiecutter templates, all internal framework unit tests
Added Acceptance test suite (tests/acceptance/plugins/test_cpex_contract.py), migration guide (docs/.../migration-to-cpex.md), Playwright smoke tests for plugin admin UI
DB migration Added on_error column to tool_plugin_bindings (with Alembic migration and check constraint)
Docs Updated plugin docs, AGENTS.md, llms guidance, plugins README, CHANGELOG (with migration guide)

Notes

  • This PR ads a compatibility layer to ensure modes are fully backwards compatible to legacy CF plugin modes.
  • A migration guide was created to help users migrate to the new plugin execution and error modes.

@araujof araujof added MUST P1: Non-negotiable, critical requirements without which the product is non-functional or unsafe and removed SHOULD P2: Important but not vital; high-value items that are not crucial for the immediate release labels May 6, 2026
Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
@brian-hussey brian-hussey self-assigned this May 6, 2026
Copy link
Copy Markdown
Collaborator

@msureshkumar88 msureshkumar88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ No unit tests for HMAC signing/verification paths

_sign_message and _verify_and_extract (mcpgateway/plugins/__init__.py:118–155) have no dedicated unit tests. These functions are the core of the security hardening added in this PR — they are exactly the paths that need regression coverage.

Missing test cases:

  1. Tampered signature_verify_and_extract must return None and log an error when sig does not match the payload:

    def test_verify_and_extract_tampered_sig(monkeypatch):
        monkeypatch.setenv("JWT_SECRET_KEY", "test-secret")
        signed = _sign_message('{"tool": "x"}')
        envelope = json.loads(signed)
        envelope["sig"] = envelope["sig"][:-1] + ("0" if envelope["sig"][-1] != "0" else "1")
        assert _verify_and_extract(json.dumps(envelope)) is None
  2. Missing payload key in envelope — a dict with sig but no payload must not raise; should return None or fall through to the plain-message path:

    def test_verify_and_extract_missing_payload_key(monkeypatch):
        monkeypatch.setenv("JWT_SECRET_KEY", "test-secret")
        bad_envelope = json.dumps({"sig": "abc123"})
        # Should not raise KeyError; return None or the raw string
        result = _verify_and_extract(bad_envelope)
        assert result is None or isinstance(result, str)
  3. Round-trip: sign → verify — happy path with key configured:

    def test_sign_verify_roundtrip(monkeypatch):
        monkeypatch.setenv("JWT_SECRET_KEY", "test-secret")
        payload = '{"tool": "y", "args": {}}'
        assert _verify_and_extract(_sign_message(payload)) == payload
  4. No key configured_sign_message returns payload unchanged, _verify_and_extract accepts both plain and envelope forms:

    def test_sign_no_key(monkeypatch):
        monkeypatch.delenv("JWT_SECRET_KEY", raising=False)
        payload = '{"tool": "z"}'
        assert _sign_message(payload) == payload
        assert _verify_and_extract(payload) == payload

These are the exact failure modes the HMAC hardening was meant to prevent. Without them, a future refactor that breaks hmac.compare_digest or the envelope parsing would pass all existing tests.

@msureshkumar88
Copy link
Copy Markdown
Collaborator

Thank you for the quick turnaround on the requested changes — all four items from the last review have been addressed:

  • cpex>=0.1.0,<0.2 upper bound added
  • PluginBindingMode.ENFORCE_IGNORE_ERROR added to the schema enum
  • invalidate_team parallelized with asyncio.gather
  • HMAC unsigned message log level escalated to WARNING when key is configured

One item remains before merge — unit tests for _sign_message and _verify_and_extract. See the latest review for the specific cases needed.

@msureshkumar88
Copy link
Copy Markdown
Collaborator

Documentation Review

Good migration guide and mode compatibility work overall. Found a few documentation gaps worth tracking — suggesting these go into a follow-up task given the merge-blocker concerns already in flight.

Findings

High

  • llms/plugins-llms.md: PromptPosthookPayload(name: str, ...) still shows the old .name field — was renamed to .prompt_id in this PR (Step 3 of migration guide).

Medium

  • migration-to-cpex.md troubleshooting: two entries say "See Step 4" but Step 4 is "Update Tool Plugin Bindings (API)" — payload fields are Step 3. Broken anchor slug too.
  • migration-to-cpex.md scaffolding section uses cpex bootstrap; every other doc (lifecycle.md, llms/plugins-llms.md, index.md) uses mcpplugins bootstrap. One should be authoritative.
  • index.md and lifecycle.md mode tables only list 3 modes (sequential, transform, disabled). New native modes (concurrent, audit, fire_and_forget) are described in the migration guide only — new users won't find them in the main reference docs.
  • index.md config schema table has no row for the new on_error field (valid values: fail, ignore, disable).

Low

  • migration-to-cpex.md checklist: pytest tests/unit/mcpgateway/plugins/ -v — the framework/ subtree was deleted by this PR; checklist should point to tests/acceptance/plugins/ or clarify what remains.
  • plugins/README.md and plugins/AGENTS.md are not in the changed files — if they contain old import paths or mode names, they may be stale.

Suggestion

The stale .name field in llms/plugins-llms.md is the only one that could actively mislead users writing plugins. The rest are reference gaps rather than correctness issues.

Worth tracking the full set in a follow-up docs issue so this PR can move forward on its current blockers (cpex tagged release, cpex-plugins freeze).

@lucarlig
Copy link
Copy Markdown
Collaborator

lucarlig commented May 6, 2026

@araujof still have references to old import:

rg -n "mcpgateway\.plugins\.framework\.external\.mcp\.server\.runtime" plugins/external
plugins/external/cedar/run-server.sh
27:    API_SERVER_SCRIPT="$(python -c 'import mcpgateway.plugins.framework.external.mcp.server.runtime as server; print(server.__file__)')"

plugins/external/clamav_server/README.md
20:- Use the gateway runtime: `mcpgateway.plugins.framework.external.mcp.server.runtime`.

plugins/external/clamav_server/run.sh
8:exec python -m mcpgateway.plugins.framework.external.mcp.server.runtime

plugins/external/opa/run-server.sh
28:    API_SERVER_SCRIPT="$(python -c 'import mcpgateway.plugins.framework.external.mcp.server.runtime as server; print(server.__file__)')"

plugins/external/llmguard/run-server.sh
27:    API_SERVER_SCRIPT="$(python -c 'import mcpgateway.plugins.framework.external.mcp.server.runtime as server; print(server.__file__)')"

Signed-off-by: Frederico Araujo <frederico.araujo@ibm.com>
@araujof
Copy link
Copy Markdown
Member Author

araujof commented May 6, 2026

Thanks @lucarlig @msureshkumar88 !

Fixed:

  1. 5 stale import paths in plugins/external/ — updated mcpgateway.plugins.framework.external.mcp.server.runtime →
    cpex.framework.external.mcp.server.runtime
  2. Stale .name field in llms/plugins-llms.md → now shows .prompt_id
  3. 2 broken anchor links in migration-to-cpex.md — pointed to Step 4, now correctly point to Step 3
  4. Inconsistent CLI command in migration-to-cpex.md — cpex bootstrap → mcpplugins bootstrap

@msureshkumar88 The comments on HMAC unit testing were already addressed: We have 9 tests covering all 4 scenarios you requested plus 5 additional edge cases.

At this point, if you could run a last end to end battery of checks, that would be awesome!
I think we can push any additional minor and doc changes into a separate PR.

cc: @brian-hussey

@araujof araujof requested a review from msureshkumar88 May 6, 2026 10:42
Copy link
Copy Markdown
Collaborator

@msureshkumar88 msureshkumar88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final E2E Battery — Approved ✅

Ran a full end-to-end check pass before approving.

Local test results

Suite Result
Full unit suite () 16,957 passed, 111 skipped, 2 xfailed
Plugin unit tests () 518 passed, 33 skipped
HMAC + acceptance (, ) 116 passed
Bandit scan () 0 issues
Ruff lint () All checks passed

Previously open items — all resolved

  • HMAC unit tests ( / ): full coverage in test_plugin_runtime_management.py — round-trip, tampered sig, tampered payload, unsigned-without-key, unsigned-when-key-configured, invalid JSON ✅
  • Stale import paths in cedar/run-server.sh, llmguard/run-server.sh, opa/run-server.sh, clamav_server/run.sh, clamav_server/README.md: all updated to cpex.framework.external.mcp.server.runtime
  • llms/plugins-llms.md → field: fixed ✅
  • Migration guide anchor links: corrected to point to Step 3 ✅
  • cpex>=0.1.0,<0.2 upper bound: present ✅
  • ENFORCE_IGNORE_ERROR in PluginBindingMode enum as deprecated alias: present ✅
  • asyncio.gather in invalidate_team (gateway_plugin_manager.py:420): implemented ✅
  • WARNING log for unsigned messages when HMAC configured (__init__.py:153): escalated ✅

Other checks

  • Alembic: single head (926d3e07d098) — clean ✅
  • CI checks currently queued (re-triggered by recent commits), not failing ✅

The framework extraction is clean — 139 files deleted, net code reduction, no regressions in the full test suite. All review blockers resolved. Good to merge.

Copy link
Copy Markdown
Collaborator

@msureshkumar88 msureshkumar88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Final E2E Battery — Approved

Ran a full end-to-end check pass before approving.

Local test results

Suite Result
Full unit suite (tests/unit/) 16,957 passed, 111 skipped, 2 xfailed
Plugin unit tests (tests/unit/mcpgateway/plugins/) 518 passed, 33 skipped
HMAC + acceptance (test_plugin_runtime_management, test_cpex_contract) 116 passed
Bandit scan (mcpgateway/plugins/) 0 issues
Ruff lint (mcpgateway/plugins/) All checks passed

Previously open items — all resolved

  • HMAC unit tests for _sign_message / _verify_and_extract: full coverage in test_plugin_runtime_management.py — round-trip, tampered sig, tampered payload, unsigned-without-key, unsigned-when-key-configured, invalid JSON ✅
  • Stale import paths in cedar/run-server.sh, llmguard/run-server.sh, opa/run-server.sh, clamav_server/run.sh, clamav_server/README.md: all updated to cpex.framework.external.mcp.server.runtime
  • llms/plugins-llms.md .name.prompt_id field: fixed ✅
  • Migration guide anchor links: corrected to point to Step 3 ✅
  • cpex>=0.1.0,<0.2 upper bound: present in pyproject.toml ✅
  • ENFORCE_IGNORE_ERROR in PluginBindingMode enum as deprecated alias: present ✅
  • asyncio.gather in invalidate_team (gateway_plugin_manager.py:420): implemented ✅
  • WARNING log for unsigned messages when HMAC configured (init.py:153): escalated ✅

Other checks

  • Alembic: single head (926d3e07d098) — clean ✅
  • CI checks currently queued (re-triggered by recent commits), not failing ✅

The framework extraction is clean — 139 files deleted, net code reduction, no regressions in the full test suite. All review blockers resolved. Good to merge.

Copy link
Copy Markdown
Member

@brian-hussey brian-hussey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved based on others' reviews.
All issues addressed. Merging.

@brian-hussey brian-hussey merged commit bc08625 into main May 6, 2026
61 checks passed
@brian-hussey brian-hussey deleted the feat/cpex branch May 6, 2026 12:51
jonpspri added a commit that referenced this pull request May 6, 2026
Updates down_revision from aa1b2c3d4e5f to 926d3e07d098. Origin/main
gained the CPEX plugin framework replacement (#3754) which introduced
two new migrations and a mergepoint, leaving our grpc_service_id
migration as a sibling head. Re-pointing to the new head returns the
chain to single-headed.

Verified single head:
  alembic heads -> w7x8y9z0a1b2 (head)
  alembic upgrade head -> clean on a fresh sqlite DB

#2854
Branch: GrpcMethodsAsTools-2854
Signed-off-by: Jonathan Springer <jps@s390x.com>
jonpspri added a commit that referenced this pull request May 6, 2026
Updates down_revision from aa1b2c3d4e5f to 926d3e07d098. Origin/main
gained the CPEX plugin framework replacement (#3754) which introduced
two new migrations and a mergepoint, leaving our grpc_service_id
migration as a sibling head. Re-pointing to the new head returns the
chain to single-headed.

Verified single head:
  alembic heads -> w7x8y9z0a1b2 (head)
  alembic upgrade head -> clean on a fresh sqlite DB

#2854
Branch: GrpcMethodsAsTools-2854
Signed-off-by: Jonathan Springer <jps@s390x.com>
jonpspri added a commit that referenced this pull request May 6, 2026
Updates down_revision from aa1b2c3d4e5f to 926d3e07d098. Origin/main
gained the CPEX plugin framework replacement (#3754) which introduced
two new migrations and a mergepoint, leaving our grpc_service_id
migration as a sibling head. Re-pointing to the new head returns the
chain to single-headed.

Verified single head:
  alembic heads -> w7x8y9z0a1b2 (head)
  alembic upgrade head -> clean on a fresh sqlite DB

#2854
Branch: GrpcMethodsAsTools-2854
Signed-off-by: Jonathan Springer <jps@s390x.com>
jonpspri added a commit that referenced this pull request May 7, 2026
Updates down_revision from aa1b2c3d4e5f to 926d3e07d098. Origin/main
gained the CPEX plugin framework replacement (#3754) which introduced
two new migrations and a mergepoint, leaving our grpc_service_id
migration as a sibling head. Re-pointing to the new head returns the
chain to single-headed.

Verified single head:
  alembic heads -> w7x8y9z0a1b2 (head)
  alembic upgrade head -> clean on a fresh sqlite DB

#2854
Branch: GrpcMethodsAsTools-2854
Signed-off-by: Jonathan Springer <jps@s390x.com>
jonpspri added a commit that referenced this pull request May 7, 2026
* feat: Add grpc_service_id to Tool schema

#2854
Branch: GrpcMethodsAsTools-2854

Signed-off-by: Gabe Goodhart <ghart@us.ibm.com>

AI-usage: full

* feat: Add tool registration and deletion for registering/deleting gRPC services

#2854
Branch: GrpcMethodsAsTools-2854

Signed-off-by: Gabe Goodhart <ghart@us.ibm.com>

AI-usage: full

* feat: Add load_file_descriptors for grpc serialized proto descriptor bytes

#2854
Branch: GrpcMethodsAsTools-2854

Signed-off-by: Gabe Goodhart <ghart@us.ibm.com>

AI-usage: full

* feat: Add gRPC tool invocation support

#2854
Branch: GrpcMethodsAsTools-2854

Signed-off-by: Gabe Goodhart <ghart@us.ibm.com>

AI-usage: full

* test: Unit tests for gRPC tool registration

#2854
Branch: GrpcMethodsAsTools-2854

Signed-off-by: Gabe Goodhart <ghart@us.ibm.com>

AI-usage: full

* style: make lint

#2854
Branch: GrpcMethodsAsTools-2854

Signed-off-by: Gabe Goodhart <ghart@us.ibm.com>

* test: Fix missing grpc_service_id in tool unit test

#2854
Branch: GrpcMethodsAsTools-2854

Signed-off-by: Gabe Goodhart <ghart@us.ibm.com>

AI-usage: full

* test: Flesh out test coverage for new gRPC code paths

#2854
Branch: GrpcMethodsAsTools-2854

Signed-off-by: Gabe Goodhart <ghart@us.ibm.com>

AI-usage: full

* fix: Better error handling for protobuf descriptor pool adding

#2854
Branch: GrpcMethodsAsTools-2854

Signed-off-by: Gabe Goodhart <ghart@us.ibm.com>

* fix: Inherit visibility from parent gRPC service when mapping to tools

#2854
Branch: GrpcMethodsAsTools-2854
AI-usage: none
Signed-off-by: Gabe Goodhart <ghart@us.ibm.com>

* fix: Fix alembic migration after rebase

#2854
Branch: GrpcMethodsAsTools-2854
AI-usage: full
Signed-off-by: Gabe Goodhart <ghart@us.ibm.com>

* fix: Repoint alembic migration parent to current head after rebase

Updates down_revision from x7h8i9j0k1l2 to aa1b2c3d4e5f, the current
head on origin/main. Several new migrations have landed since this PR
was last rebased, so the previous parent is no longer the head.

Verified single head: alembic heads -> w7x8y9z0a1b2 (head)

#2854
Branch: GrpcMethodsAsTools-2854
Signed-off-by: Jonathan Springer <jps@s390x.com>

* test: Add grpc_service_id=None to TestRustMcpExecutionPlan SimpleNamespace mocks

Two tests added to TestRustMcpExecutionPlan after this PR was last
rebased construct tool mocks via SimpleNamespace. The PR's change to
_build_tool_cache_payload reads tool.grpc_service_id, so those mocks
now AttributeError. Mirrors b576ec4's earlier fix for the same
pattern in MagicMock(spec=DbTool) helpers.

Affected:
- TestRustMcpExecutionPlan.test_prepare_rust_mcp_tool_execution_uses_live_gateway_auth_fields_for_loaded_tools
- TestRustMcpExecutionPlan.test_prepare_rust_mcp_tool_execution_uses_live_gateway_string_auth_values

#2854
Branch: GrpcMethodsAsTools-2854
Signed-off-by: Jonathan Springer <jps@s390x.com>

* style: make black isort

Tightens whitespace and import grouping per project lint config:
- mcpgateway/services/grpc_service.py: black removes a stray blank line
- tests/unit/mcpgateway/{services/test_grpc_service,services/test_tool_service,test_translate_grpc}.py: isort regroups stdlib/third-party/first-party/local imports

#2854
Branch: GrpcMethodsAsTools-2854
Signed-off-by: Jonathan Springer <jps@s390x.com>

* fix(grpc): use protobuf 6.x kwarg in MessageToDict and isolate descriptor pool

Two issues addressed in mcpgateway/translate_grpc.py:

1. Pre-existing runtime bug: invoke()/invoke_streaming() called
   json_format.MessageToDict(..., including_default_value_fields=True),
   the parameter name from protobuf 4.x. The repo pins protobuf>=6.33.6
   (see pyproject.toml), where the parameter was renamed to
   always_print_fields_with_no_presence. Calls raised TypeError at
   runtime, breaking gRPC tool invocation. Renamed the kwarg and
   replaced the # pylint: disable=unexpected-keyword-arg suppression
   with an anti-regression comment.

2. Descriptor pool poisoning: GrpcEndpoint shared
   descriptor_pool.Default(), the process-wide singleton. Reflected
   FileDescriptorProto from one (untrusted) upstream service could
   leak across requests and cause symbol collisions or type confusion
   in subsequent calls. Switched to a per-endpoint
   descriptor_pool.DescriptorPool(); MessageFactory now binds to that
   private pool.

Updated tests/unit/mcpgateway/test_translate_grpc.py monkeypatches to
expose DescriptorPool() instead of Default() and MessageFactory(pool=...)
instead of MessageFactory().

Closes review: B2, B11

#2854
Branch: GrpcMethodsAsTools-2854
Signed-off-by: Jonathan Springer <jps@s390x.com>

* fix(grpc): security and robustness hardening from review

Addresses BLOCKING findings from a multi-agent code review of PR #3202:

Security:
* Reject local-only gRPC schemes (unix:, unix-abstract:, vsock:, fd:)
  in _validate_grpc_target since they bypass the IP-based SSRF model.
  Also normalize dns:/// and ipv4:/ipv6: name-resolver prefixes and
  parse bracketed IPv6 hosts correctly so the host-extraction step
  cannot be tricked. (review B1)
* Add _enforce_descriptor_limits to bound reflected
  FileDescriptorProto count, per-blob size, and aggregate size before
  storing or loading. Hardcoded constants (not settings) so a config
  change cannot silently weaken the DoS defense. (review B3)
* Add _validate_reflected_tool_name that runs reflected tool names
  through SecurityValidator.validate_tool_name() before persisting.
  Reflected names from upstream gRPC servers no longer bypass the
  injection / length / character checks applied to user-registered
  tools. (review B4)
* Strip metadata pseudo-keys (anything starting with '_') when
  copying discovered_services into endpoint._services so the
  _file_descriptors entry can never be confused with a service.
* Decode stored descriptors with base64.b64decode(..., validate=True)
  to fail fast on tampered DB content.

Robustness / Layer 1 invariants:
* _sync_tools_from_reflection now runs each method through a
  per-tool try/except so a single bad method cannot abort the
  entire reflection sync. Counts created / updated / failed.
  (review B9)
* _sync_tools_from_reflection now propagates parent service
  visibility, team_id, and owner_email to existing reflected tools,
  not just to newly created ones. Closes a Layer 1 token-scoping gap
  where a public->team visibility change on the parent service was
  silently ignored on already-discovered tools. (review B5)

Operational:
* invoke_method gains a timeout parameter (default settings.tool_timeout)
  and wraps endpoint.start() / endpoint.invoke() in asyncio.wait_for so
  a slow upstream cannot tie up a worker indefinitely. (review B6)
* tool_service.invoke_tool gRPC branch wraps the call in
  asyncio.wait_for, surfaces ToolTimeoutError + post-invoke timeout
  hook (matches A2A branch), preserves CancelledError instead of
  swallowing it, and logs with %-style + exc_info=True. (review B6/B7/B8)
* invoke_method preserves CancelledError, logs with %-style +
  exc_info=True, re-raises GrpcServiceError/GrpcServiceNotFoundError
  unwrapped instead of double-wrapping them. (review B8)

#2854
Branch: GrpcMethodsAsTools-2854
Signed-off-by: Jonathan Springer <jps@s390x.com>

* refactor(grpc): drop redundant inline settings import

The inline 'from mcpgateway.config import settings' in
_validate_grpc_target was made redundant by hoisting the same import
to module top in the security hardening commit.

Branch: GrpcMethodsAsTools-2854
Signed-off-by: Jonathan Springer <jps@s390x.com>

* test(grpc): cover security-hardening helpers and Layer 1 invariants

Adds two test classes exercising the helpers introduced by the
security/robustness hardening commit:

TestSecurityHardening:
* _validate_grpc_target rejects unix:/unix-abstract:/vsock:/fd: schemes
* _validate_grpc_target normalizes dns:///, dns://, dns:, ipv4:, ipv6:
  name-resolver prefixes
* _validate_grpc_target accepts bracketed IPv6 literals and rejects
  malformed bracket syntax
* _enforce_descriptor_limits enforces count, per-blob, and aggregate
  size caps; happy path passes within bounds
* _validate_reflected_tool_name rejects empty / over-length / injection
  patterns and accepts proto-style identifiers

TestVisibilityPropagation:
* _sync_tools_from_reflection propagates parent service visibility,
  team_id, and owner_email to existing reflected tools (Layer 1
  invariant).
* _sync_tools_from_reflection isolates per-method failures: a method
  with an invalid name is skipped while the rest of the sync proceeds
  (B9 regression test).

All 13 new tests pass; the broader gRPC + tool_service + translate_grpc
suite is green at 520 tests.

#2854
Branch: GrpcMethodsAsTools-2854
Signed-off-by: Jonathan Springer <jps@s390x.com>

* fix(grpc): close remaining review gaps from iteration-2 oracle pass

Addresses BLOCKING items the second-pass review identified as still
present after the first round of fixes.

* B1 (DNS resolution): _validate_grpc_target now delegates the
  hostname/IP-network/DNS policy to SecurityValidator._validate_ssrf so
  hostnames like metadata.google.internal are resolved and the resolved
  IPs go through the same blocklist as HTTP targets. Removes the
  duplicate hand-rolled IP-network logic that previously lived inline.

* B5 (visibility on update): GrpcService.update_service now snapshots
  visibility / team_id / owner_email before applying the update and
  bulk-updates every child Tool with grpc_service_id == service.id in
  the same transaction whenever any of those fields change. Closes the
  Layer 1 token-scoping gap where a public->team change on the parent
  service was silently ignored on already-discovered tools.

* B6 (real gRPC deadlines): GrpcEndpoint.start, _discover_services,
  and _discover_service_details now accept a timeout kwarg that is
  threaded into ServerReflectionInfo, and GrpcEndpoint.invoke binds
  the deadline onto the underlying unary_unary call so a slow upstream
  cannot keep an executor thread alive after asyncio.wait_for cancels
  the wrapping coroutine. invoke_method passes the effective tool
  timeout into both endpoint.start and endpoint.invoke.

* B7 (cancellation propagation in outer except BaseException): adds
  except asyncio.CancelledError: raise immediately before the three
  outer except BaseException as e blocks in tool_service.invoke_tool
  so a cancellation is never wrapped as a ToolInvocationError.

* B12 (protobuf 6.x compatibility): MessageFactory.GetPrototype was
  removed in protobuf >= 5.x. Both the unary and streaming invocation
  paths in translate_grpc now call message_factory.GetMessageClass(...)
  instead. Updated the corresponding unit-test mocks.

* Cleanup: drops unused import ipaddress that became dead after
  the SecurityValidator delegation.

Test mocks updated to match the new signatures (timeout kwarg on the
populate_service / _populate callbacks; GetMessageClass at the module
level instead of MessageFactory.GetPrototype).

Lint clean (black, isort, ruff E3/E4/E7/E9/F/D1). 520 targeted tests
pass.

#2854
Branch: GrpcMethodsAsTools-2854
Signed-off-by: Jonathan Springer <jps@s390x.com>

* fix(grpc): preserve timeout=0 fail-fast semantics in deadline ternaries

The 'if timeout' ternary in three reflection/unary call sites silently
drops timeout=0, but gRPC accepts timeout=0 as the explicit 'fail
immediately' deadline. Switched to 'if timeout is not None' so a
caller-supplied 0.0 reaches the underlying gRPC call with the right
semantics.

Found in iteration-3 oracle review (final pass, no new BLOCKING items).

Branch: GrpcMethodsAsTools-2854
Signed-off-by: Jonathan Springer <jps@s390x.com>

* fix(grpc): apply non-blocking review nits flagged across the three review passes

Bundle of low-risk, non-blocking improvements that surfaced during the
deep review iterations. Each item below was explicitly classified as
fix-now in the review summary; broader / design-dependent items were
filed as follow-ups under #4612 and #4613.

* Schema parity (#1): add grpc_service_id to ToolRead in schemas.py
  alongside the existing gateway_id field so API consumers can identify
  gRPC-discovered tools (review S2.4 / TD1).

* db.py cleanup (#2): drop redundant nullable=True from
  Tool.grpc_service_id; Mapped[Optional[str]] already implies it and
  the sibling gateway_id column omits it (review TD2).

* translate_grpc.load_file_descriptors (#3): widen the parameter type
  from List[bytes] to Sequence[bytes] and reject a single bytes object
  passed by mistake. Without the guard Python would silently iterate
  byte-by-byte (review TD3).

* translate_grpc descriptor pool conflict (#4): replace the inaccurate
  'no-op if already added' comment with an explicit TypeError handler.
  protobuf raises TypeError when a file with the same name has
  conflicting content; the existing descriptor stays authoritative
  (review S3.3).

* test_sync_tools_removes_stale_tools (#5): replace the brittle
  string-match assertion ('DELETE' in str(call)) with an explicit
  call_count == 4 (1 select + 3 deletes), which no longer depends on
  the SQLAlchemy Delete object repr (review S2.6).

* translate_grpc close() (#6): convert the f-string log to lazy
  %-style for consistency with the rest of this PR's logging
  (review N2.1).

* grpc_service _sync_tools_from_reflection (#7): document why
  input_schema['properties'] is empty by design — gRPC arg shape is
  validated at the protobuf invocation layer, not the MCP tool-call
  layer; the actual proto types live in the x-grpc-* extensions
  (review N2.2).

* TestInvokeMethodGuards (#8): add 5 edge-case tests covering
  invoke_method paths the previous suite did not exercise:
  - service-not-found  -> GrpcServiceNotFoundError
  - disabled service   -> GrpcServiceError('is disabled')
  - invalid method format (no dot) -> GrpcServiceError
  - _validate_grpc_target spy called with service.target
  - _validate_tls_path spy called for both cert and key paths

525 targeted tests pass (was 520). Lint clean.

Branch: GrpcMethodsAsTools-2854
Signed-off-by: Jonathan Springer <jps@s390x.com>

* style: clean lint cycle (make autoflake/isort/black/ruff/bandit/interrogate/pylint)

Remaining fixes the project lint cycle surfaced after the fix-now bundle landed:

* isort: re-sort sibling imports in test_grpc_service.py
  (_enforce_descriptor_limits before _GRPC_MAX_*; case-insensitive order).

* ruff PLW0108 (unnecessary lambda) in test_translate_grpc.py: align the
  4 mocks I added with the 7 pre-existing patterns in the same file
  (lambda **_kw: MagicMock() / lambda *_a, **_kw: MagicMock()).

* ruff RET501 in test_grpc_service.py::tls_spy: drop the redundant
  trailing 'return None'.

* pylint no-member on the alembic migration: add the project-standard
  '# pylint: disable=no-member' header that 9 other migrations use to
  silence alembic.op's dynamic-attribute false positives.

* pylint try-except-raise on tool_service.invoke_tool gRPC branch:
  the 'except asyncio.CancelledError: raise' clause LOOKS like a no-op
  but exists specifically so the LATER 'except Exception' cannot swallow
  cancellation (PR #3202 review B7). Added an inline disable plus a
  comment citing B7 so the next lint sweep doesn't delete the
  anti-regression code.

Verified after fix:
  make autoflake / isort / black -> clean
  make ruff (E3,E4,E7,E9,F,D1)  -> All checks passed
  make bandit                   -> 0 issues, 18022 LoC
  make interrogate              -> 100% docstring coverage
  make pylint                   -> 10.00/10 on all 6 production files
  pytest (targeted gRPC suite)  -> 525/525 passing

Branch: GrpcMethodsAsTools-2854
Signed-off-by: Jonathan Springer <jps@s390x.com>

* fix: Re-parent alembic migration to current head after second rebase

Updates down_revision from aa1b2c3d4e5f to 926d3e07d098. Origin/main
gained the CPEX plugin framework replacement (#3754) which introduced
two new migrations and a mergepoint, leaving our grpc_service_id
migration as a sibling head. Re-pointing to the new head returns the
chain to single-headed.

Verified single head:
  alembic heads -> w7x8y9z0a1b2 (head)
  alembic upgrade head -> clean on a fresh sqlite DB

#2854
Branch: GrpcMethodsAsTools-2854
Signed-off-by: Jonathan Springer <jps@s390x.com>

* fix(grpc): restore reserved/multicast guard and update test_grpc_service_no_grpc

Two related fixes for the no-grpc test file that surfaced after rebase:

1. Restore is_reserved / is_multicast pre-check in _validate_grpc_target.
   The earlier delegation to SecurityValidator._validate_ssrf inadvertently
   dropped this guard because the shared validator only checks
   blocked-networks / localhost / private. IP literals like 224.0.0.1
   (multicast) or 0.0.0.0 (reserved) would have slipped past unless they
   were also in ssrf_blocked_networks. The restored check excludes
   loopback so ::1 (which Python flags as both is_loopback AND
   is_reserved) still passes through the localhost gate as intended.

2. Update test_grpc_service_no_grpc.py:
   - Add timeout=None kwarg to the three FakeEndpoint.start() and
     invoke() methods so they accept the per-RPC deadline added in the
     security/robustness hardening commit.
   - Update test_validate_grpc_target_enforces_ssrf_rules regex
     patterns to match the SecurityValidator messages reached via
     delegation (blocked hostname X, localhost address which is
     blocked, private network address which is blocked).

All 4 gRPC test files pass: 528 / 528.

#2854
Branch: GrpcMethodsAsTools-2854
Signed-off-by: Jonathan Springer <jps@s390x.com>

* test: increase diff coverage for gRPC PR from 92% to 100% on grpc_service+translate_grpc

Adds 11 targeted tests + a decision-record comment to address the diff-coverage
report that flagged 18 previously-uncovered lines:

  grpc_service.py:    95.7% -> 100%  (7/7 covered)
  translate_grpc.py:  91.7% -> 100%  (3/3 covered)
  tool_service.py:    69.2% -> 62%   (5/8 covered + 3 documented)

New tests in tests/unit/mcpgateway/services/test_grpc_service.py:

* test_validate_grpc_target_empty_string                        -> line 134
* test_invoke_method_propagates_cancelled_error                 -> line 1021
* test_invoke_method_re_raises_timeout                          -> lines 1023-1024
* test_invoke_method_re_raises_grpc_service_error_unwrapped     -> line 1026
* test_update_service_propagates_visibility_to_child_tools      -> lines 503-504

New tests in tests/unit/mcpgateway/test_translate_grpc.py:

* test_load_file_descriptors_rejects_single_bytes               -> line 409
* test_load_file_descriptors_skips_pool_conflict                -> lines 420, 425

New tests in tests/unit/mcpgateway/services/test_tool_service.py:

* test_invoke_grpc_tool_propagates_cancellation                 -> line 5847
* test_invoke_grpc_tool_timeout_raises_tool_timeout_error       -> lines 5849-5850, 5852

Restored a regression: the earlier B1 refactor (delegating to
SecurityValidator._validate_ssrf) inadvertently dropped the is_reserved /
is_multicast guard, since SecurityValidator only checks blocked-networks /
localhost / private. Restored the local guard with an is_loopback exclusion
so IPv6 ::1 (which Python flags as both is_loopback AND is_reserved)
still passes through the localhost gate.

Coverage decision-record left in test_tool_service.py for the 3 remaining
B7 anti-regression lines (5446 REST, 5632 MCP, 5851 gRPC timeout-with-pm):
they are byte-identical to the gRPC variant at line 5847 (which is fully
exercised). Equivalent REST/MCP tests require coaxing CancelledError
through asyncio.wait_for, which Python's event loop converts to TimeoutError
in some states. Protecting one branch is sufficient to detect a regression
that would affect all three.

541 tests passing, ruff/black/isort/pylint clean.

#2854
Branch: GrpcMethodsAsTools-2854
Signed-off-by: Jonathan Springer <jps@s390x.com>

* fix: re-parent alembic migration onto current head + restore interrogate 100%

Two CI failures triggered by main moving forward during this PR's review:

* alembic multi-head: PR #4501 ("add ondelete=CASCADE to FK constraints")
  introduced 9fb98535724d as a sibling of our w7x8y9z0a1b2, both parented
  to 926d3e07d098. Re-parents w7x8y9z0a1b2 -> 9fb98535724d so the chain
  has a single head again and bootstrap_db / SQLite+Postgres upgrade
  validation pass. Verified single head with alembic heads.

* interrogate --fail-under=100: three nested helper functions had no
  docstrings, dropping the package to 99.9%. One is mine
  (GrpcEndpoint.invoke._call introduced by the gRPC timeout fix in
  PR #3202 review B6); the other two (_user_obj_to_dict._iso,
  _user_dict_to_obj._dt in services/email_auth_service.py) landed
  on main via PR #4595 and were already failing main's CI. All three
  added one-line docstrings.

Cascade fixed by these two changes:
  - SQLite + PostgreSQL Fresh/Upgrade (alembic head check)
  - pytest (py3.12) (bootstrap_db on alembic head check)
  - playwright-ci-smoke (depends on bootstrap)
  - sql-sanitizer E2E (depends on bootstrap)
  - interrogate (mcpgateway) (docstring coverage)
  - Run pre-commit hooks (interrogate is a pre-commit hook)

The 2 email_auth_service docstrings are technically out of scope for
this PR but the local lint cycle scoping I had been using
(make interrogate TARGET=<my-files>) didn't catch them. Folding
them in here unblocks PR-level CI rather than waiting for a separate
fix to main.

Verified locally:
  alembic heads      -> single head w7x8y9z0a1b2
  interrogate        -> 100% on full mcpgateway/
  black --check      -> clean
  ruff check         -> clean
  pylint             -> 10.00/10
  targeted gRPC tests -> 541 passing

#2854
Branch: GrpcMethodsAsTools-2854
Signed-off-by: Jonathan Springer <jps@s390x.com>

---------

Signed-off-by: Gabe Goodhart <ghart@us.ibm.com>
Signed-off-by: Jonathan Springer <jps@s390x.com>
Co-authored-by: Gabe Goodhart <ghart@us.ibm.com>
Co-authored-by: Jonathan Springer <jps@s390x.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

design Architecture and Design enhancement New feature or request MUST P1: Non-negotiable, critical requirements without which the product is non-functional or unsafe plugins

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE]: Replace Internal Plugin Framework with CPEX (standalone plugin framework package)

7 participants