Update testing docs, copilot docs and minor fix to servers/ also offering a servers (no /) endpoint#6
Merged
crivetimihai merged 1 commit intomainfrom May 28, 2025
Conversation
…ring a servers (no /) endpoint
vk-playground
pushed a commit
to vk-playground/mcp-context-forge
that referenced
this pull request
Sep 14, 2025
Update testing docs, copilot docs and minor fix to servers/ also offering a servers (no /) endpoint Signed-off-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
Update testing docs, copilot docs and minor fix to servers/ also offering a servers (no /) endpoint
vk-playground
pushed a commit
to vk-playground/mcp-context-forge
that referenced
this pull request
Sep 16, 2025
Update testing docs, copilot docs and minor fix to servers/ also offering a servers (no /) endpoint Signed-off-by: Vicky Kuo <vicky.kuo@ibm.com>
65 tasks
hughhennelly
added a commit
to hughhennelly/mcp-context-forge
that referenced
this pull request
Feb 12, 2026
1. Fix broken imports (Issue #1): - Change from ..database to ..db - Fix unified_pdp imports to use plugins.unified_pdp - Update in routes, services, schemas, and tests 2. Register sandbox router in main.py (Issue IBM#2): - Add import and app.include_router call 3. Fix XSS vulnerability (Issue IBM#3): - Replace f-string HTML with Jinja2 template - Create sandbox_simulate_results.html template - Add Request parameter for template access 4. Add authentication (Issue IBM#4): - Add Depends(get_current_user) to simulate endpoint 5. Remove scratch files (Issue IBM#5): - Delete sandbox_header.txt and sandbox_new_header.txt 6. Resolve schemas conflict (Issue IBM#6): - Merge schemas/sandbox.py into schemas.py - Remove conflicting schemas/ directory - Update imports in routes and services All changes tested and ready for review. Related to IBM#2226 Signed-off-by: hughhennelly <hughhennelly06@gmail.com>
yiannis2804
added a commit
to yiannis2804/mcp-context-forge
that referenced
this pull request
Feb 19, 2026
) Address code review suggestion from @jonpspri: Problem: Subject, Resource, Context, and AccessDecision used manual __init__ methods with mutable default arguments and no type validation. Solution: - Converted all 4 data models to @DataClass - Used field(default_factory=list/dict) for mutable defaults - Added __post_init__ to Context for default timestamp - Renamed Resource parameters: resource_type→type, resource_id→id Benefits: - Type validation via dataclass - Immutable defaults (no mutable default argument bugs) - Cleaner, more Pythonic code - Better serialization support - Protection against common pitfalls All 262 tests passing with dataclass models. Related: PR IBM#2682 Phase 1 Code Review Item IBM#6 Signed-off-by: yiannis2804 <yiannis2804@gmail.com>
yiannis2804
added a commit
to yiannis2804/mcp-context-forge
that referenced
this pull request
Feb 19, 2026
Address code review suggestions from @jonpspri (minor items): 1. ✅ Lazy logging format: - Changed logger.debug(f'msg: {var}') to logger.debug('msg: %s', var) - Avoids f-string evaluation when debug logging is disabled - Applied to _log_decision and check_access methods 2. ✅ Documented per-request instantiation: - Added NOTE about PolicyEngine(db) created on every request - Acceptable for Phase 1 (stateless, simple) - Can be optimized in Phase 2+ with caching/pooling 3. ✅ Improved test DB mocking: - Changed db_session fixture from next(get_db()) to MagicMock - More portable unit tests without real DB dependency - Cleaner test isolation Additional fixes: - Updated all test Resource() calls to use type= and id= parameters - Updated all test AccessDecision() calls to use resource_type= and resource_id= - Maintains consistency after dataclass conversion in Issue IBM#6 All 21 PolicyEngine tests passing. Related: PR IBM#2682 Phase 1 Code Review Item IBM#9 Signed-off-by: yiannis2804 <yiannis2804@gmail.com>
crivetimihai
pushed a commit
that referenced
this pull request
Feb 24, 2026
Address code review suggestion from @jonpspri: Problem: Subject, Resource, Context, and AccessDecision used manual __init__ methods with mutable default arguments and no type validation. Solution: - Converted all 4 data models to @DataClass - Used field(default_factory=list/dict) for mutable defaults - Added __post_init__ to Context for default timestamp - Renamed Resource parameters: resource_type→type, resource_id→id Benefits: - Type validation via dataclass - Immutable defaults (no mutable default argument bugs) - Cleaner, more Pythonic code - Better serialization support - Protection against common pitfalls All 262 tests passing with dataclass models. Related: PR #2682 Phase 1 Code Review Item #6 Signed-off-by: yiannis2804 <yiannis2804@gmail.com>
crivetimihai
pushed a commit
that referenced
this pull request
Feb 24, 2026
Address code review suggestions from @jonpspri (minor items): 1. ✅ Lazy logging format: - Changed logger.debug(f'msg: {var}') to logger.debug('msg: %s', var) - Avoids f-string evaluation when debug logging is disabled - Applied to _log_decision and check_access methods 2. ✅ Documented per-request instantiation: - Added NOTE about PolicyEngine(db) created on every request - Acceptable for Phase 1 (stateless, simple) - Can be optimized in Phase 2+ with caching/pooling 3. ✅ Improved test DB mocking: - Changed db_session fixture from next(get_db()) to MagicMock - More portable unit tests without real DB dependency - Cleaner test isolation Additional fixes: - Updated all test Resource() calls to use type= and id= parameters - Updated all test AccessDecision() calls to use resource_type= and resource_id= - Maintains consistency after dataclass conversion in Issue #6 All 21 PolicyEngine tests passing. Related: PR #2682 Phase 1 Code Review Item #9 Signed-off-by: yiannis2804 <yiannis2804@gmail.com>
crivetimihai
pushed a commit
that referenced
this pull request
Feb 24, 2026
1. Fix broken imports (Issue #1): - Change from ..database to ..db - Fix unified_pdp imports to use plugins.unified_pdp - Update in routes, services, schemas, and tests 2. Register sandbox router in main.py (Issue #2): - Add import and app.include_router call 3. Fix XSS vulnerability (Issue #3): - Replace f-string HTML with Jinja2 template - Create sandbox_simulate_results.html template - Add Request parameter for template access 4. Add authentication (Issue #4): - Add Depends(get_current_user) to simulate endpoint 5. Remove scratch files (Issue #5): - Delete sandbox_header.txt and sandbox_new_header.txt 6. Resolve schemas conflict (Issue #6): - Merge schemas/sandbox.py into schemas.py - Remove conflicting schemas/ directory - Update imports in routes and services All changes tested and ready for review. Related to #2226 Signed-off-by: hughhennelly <hughhennelly06@gmail.com>
aidbutlr
referenced
this pull request
in aidbutlr/mcp-context-forge
Mar 3, 2026
CYFR-280 adding first smoke tests
10 tasks
5 tasks
10 tasks
3 tasks
jonpspri
added a commit
that referenced
this pull request
May 6, 2026
…view 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>
jonpspri
added a commit
that referenced
this pull request
May 6, 2026
…view 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>
jonpspri
added a commit
that referenced
this pull request
May 6, 2026
…view 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>
jonpspri
added a commit
that referenced
this pull request
May 6, 2026
…view 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>
10 tasks
jonpspri
added a commit
that referenced
this pull request
May 7, 2026
…view 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>
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.