Use async context manager for initializing gateway#7
Merged
crivetimihai merged 1 commit intomainfrom May 28, 2025
Merged
Conversation
vk-playground
pushed a commit
to vk-playground/mcp-context-forge
that referenced
this pull request
Sep 14, 2025
Use async context manager for initializing gateway 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
Use async context manager for initializing gateway
vk-playground
pushed a commit
to vk-playground/mcp-context-forge
that referenced
this pull request
Sep 16, 2025
Use async context manager for initializing gateway Signed-off-by: Vicky Kuo <vicky.kuo@ibm.com>
4 tasks
yiannis2804
added a commit
to yiannis2804/mcp-context-forge
that referenced
this pull request
Feb 19, 2026
…BM#7) Address code review suggestion from @jonpspri: Problem: The _has_permission static method handles *, admin.*, and exact matches but had no dedicated tests covering edge cases. Solution: - Added 5 comprehensive wildcard permission tests - Tests cover: exact match, *, namespace.*, multiple permissions - Discovered and fixed bug: admin.* incorrectly matched 'admin' Bug Fixed: - admin.* should only match admin.SOMETHING (e.g., admin.system_config) - It should NOT match just 'admin' (no dot) - Fixed by removing 'required == prefix' check - Now correctly requires dot: required.startswith(prefix + '.') Test Coverage: ✅ test_exact_match - exact permission matching ✅ test_wildcard_star_matches_all - * matches everything ✅ test_namespace_wildcard - admin.* matches admin.anything ✅ test_wildcard_does_not_match_namespace_only - admin.* ≠ admin ✅ test_multiple_permissions - combined permission lists All 21 PolicyEngine tests passing (16 existing + 5 new). Related: PR IBM#2682 Phase 1 Code Review Item IBM#7 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: The _has_permission static method handles *, admin.*, and exact matches but had no dedicated tests covering edge cases. Solution: - Added 5 comprehensive wildcard permission tests - Tests cover: exact match, *, namespace.*, multiple permissions - Discovered and fixed bug: admin.* incorrectly matched 'admin' Bug Fixed: - admin.* should only match admin.SOMETHING (e.g., admin.system_config) - It should NOT match just 'admin' (no dot) - Fixed by removing 'required == prefix' check - Now correctly requires dot: required.startswith(prefix + '.') Test Coverage: ✅ test_exact_match - exact permission matching ✅ test_wildcard_star_matches_all - * matches everything ✅ test_namespace_wildcard - admin.* matches admin.anything ✅ test_wildcard_does_not_match_namespace_only - admin.* ≠ admin ✅ test_multiple_permissions - combined permission lists All 21 PolicyEngine tests passing (16 existing + 5 new). Related: PR #2682 Phase 1 Code Review Item #7 Signed-off-by: yiannis2804 <yiannis2804@gmail.com>
aidbutlr
referenced
this pull request
in aidbutlr/mcp-context-forge
Mar 3, 2026
CYFR-280 Enable multi-arch builds
10 tasks
MohanLaksh
added a commit
that referenced
this pull request
Apr 8, 2026
This commit addresses valid concerns raised in PR review: 1. Fix class docstring showing old API (madhu-mohan-jaishankar) - Updated ObservabilityService class docstring to show correct API - Removed db parameter from examples (old: start_trace(db, name)) - Added clarification about write vs query methods - Examples now demonstrate correct usage with proper parameters 2. Fix context manager atomicity (msureshkumar88 issue #5) - Added optional obs_db parameter to add_event() method - Context managers now pass their session to add_event() in error paths - Ensures atomic commit of span updates and error events together - Prevents orphaned events when span update fails - Updated all three context managers: * trace_span() at line 644 * trace_tool_invocation() at line 757 * trace_a2a_request() at line 1112 3. Document connection pool sizing (msureshkumar88 issue #7) - Added Connection Pool Sizing section to AGENTS.md - Explains 4-6 sessions per traced request pattern - Documents default pool (200+10 = 210 connections) - Supports ~35 concurrent traced requests by default - Guidance for high-traffic deployments (>50 req/sec) - Note about SQLite 50-connection cap Implementation details: - add_event() now accepts optional obs_db: Optional[Session] = None - When obs_db is provided, uses caller's session (owned = False) - When obs_db is None, creates independent session (owned = True) - Only closes session if owned is True - All existing tests pass (87 observability service tests, 23 middleware tests) Tests: All 87 observability service tests passing All 23 observability middleware tests passing 100% differential coverage maintained Addresses review feedback from: - @madhu-mohan-jaishankar (approved with comments) - @msureshkumar88 (issues #5 and #7 from detailed review) Signed-off-by: Mohan Lakshmaiah <mohan.economist@gmail.com>
brian-hussey
pushed a commit
that referenced
this pull request
Apr 8, 2026
This commit addresses valid concerns raised in PR review: 1. Fix class docstring showing old API (madhu-mohan-jaishankar) - Updated ObservabilityService class docstring to show correct API - Removed db parameter from examples (old: start_trace(db, name)) - Added clarification about write vs query methods - Examples now demonstrate correct usage with proper parameters 2. Fix context manager atomicity (msureshkumar88 issue #5) - Added optional obs_db parameter to add_event() method - Context managers now pass their session to add_event() in error paths - Ensures atomic commit of span updates and error events together - Prevents orphaned events when span update fails - Updated all three context managers: * trace_span() at line 644 * trace_tool_invocation() at line 757 * trace_a2a_request() at line 1112 3. Document connection pool sizing (msureshkumar88 issue #7) - Added Connection Pool Sizing section to AGENTS.md - Explains 4-6 sessions per traced request pattern - Documents default pool (200+10 = 210 connections) - Supports ~35 concurrent traced requests by default - Guidance for high-traffic deployments (>50 req/sec) - Note about SQLite 50-connection cap Implementation details: - add_event() now accepts optional obs_db: Optional[Session] = None - When obs_db is provided, uses caller's session (owned = False) - When obs_db is None, creates independent session (owned = True) - Only closes session if owned is True - All existing tests pass (87 observability service tests, 23 middleware tests) Tests: All 87 observability service tests passing All 23 observability middleware tests passing 100% differential coverage maintained Addresses review feedback from: - @madhu-mohan-jaishankar (approved with comments) - @msureshkumar88 (issues #5 and #7 from detailed review) Signed-off-by: Mohan Lakshmaiah <mohan.economist@gmail.com>
brian-hussey
added a commit
that referenced
this pull request
Apr 8, 2026
…ons (#4050) * feat: implement separate observability session pattern (best-effort) Observability write operations now use independent database sessions that commit immediately on a best-effort basis, separate from main request transactions. This ensures observability data persists even when the main request fails, providing visibility into partial failures. Implementation details: - Observability write methods (start_trace, end_trace, start_span, end_span, add_event, record_metric, record_token_usage, delete_old_traces) now create their own independent sessions using SessionLocal() instead of accepting a db parameter from the caller - Context managers (trace_span, trace_tool_invocation, trace_a2a_request) create and manage a single session for the full span lifecycle - Query methods still accept db: Session parameter for RBAC/token scoping - ObservabilityMiddleware no longer creates or manages request.state.db - Each observability operation creates its own short-lived session that commits immediately in a try/except/finally block - Updated all callers across resource_service, prompt_service, tool_service, and instrumentation/sqlalchemy to use new API (removed db parameters) - Added comprehensive test coverage verifying separate session behavior - Fixed pre-existing bugs in add_event() and record_metric() where db.refresh() was called after commit failure Benefits: - Observability data persists independently of main request transaction success - Provides visibility into partial failures and error states - Follows pattern established in instrumentation/sqlalchemy.py (lines 58-87) - Reduces transaction contention between observability and business logic Trade-offs: - NOT atomic with main request transaction (intentional design choice) - Traces may show "in progress" or partial states for failed requests - Provides eventual consistency rather than strict consistency Documentation: - Added "Observability Transaction Behavior" section to AGENTS.md explaining the separate session pattern, implementation details, and security notes - Updated module docstrings with session management details Tests: 96 observability tests passing, full suite 16,673 tests passing Pylint: 10.00/10 rating maintained Closes #3883 Signed-off-by: Mohan Lakshmaiah <mohan.economist@gmail.com> * fix: add type annotations to observability context managers Add proper return type annotations to context manager methods: - trace_span() -> Generator[str, None, None] - trace_tool_invocation() -> Generator[Tuple[Optional[str], Dict[str, Any]], None, None] - trace_a2a_request() -> Generator[Tuple[Optional[str], Dict[str, Any]], None, None] These annotations improve type safety and make the return values explicit. The trace_span context manager yields just the span_id string, while trace_tool_invocation and trace_a2a_request yield a tuple of (span_id, result_dict) for capturing operation results. Tests: 96 observability tests passing Pylint: 10.00/10 rating maintained Signed-off-by: Mohan Lakshmaiah <mohan.economist@gmail.com> * test: Add coverage tests for observability session close exception handling Add 11 new test functions to achieve 100% coverage of exception handling paths in observability_service.py. These tests verify that all write methods and context managers handle session close failures gracefully: - test_start_trace_session_close_failure - test_end_trace_session_close_failure - test_start_span_session_close_failure - test_start_span_with_commit_true - test_end_span_session_close_failure - test_add_event_session_close_failure - test_record_metric_session_close_failure - test_delete_old_traces_session_close_failure - test_trace_span_context_manager_session_close_failure - test_trace_tool_invocation_context_manager_session_close_failure - test_trace_a2a_request_context_manager_session_close_failure Each test mocks the session close() method to raise an exception and verifies that the operation completes successfully despite the close failure. This covers the previously uncovered lines in finally blocks where session close exceptions are caught and logged. Resolves CI/CD coverage requirement for observability_service.py. Signed-off-by: Mohan Lakshmaiah <mohan.economist@gmail.com> * fix: complete separate session migration for delete_old_traces and record_transport_activity - Fix runtime TypeError in cleanup_old_traces router: delete_old_traces() no longer accepts db parameter (missed call site from #3883) - Remove unused db parameter from record_transport_activity() for API consistency with other write methods - Update docstring and doctest for delete_old_traces call in router - Update test calls to match new record_transport_activity signature - Add 3 tests for 100% differential coverage: error-path exception handlers in trace_tool_invocation, trace_a2a_request, and session close failure in record_token_usage Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * fix: address review feedback for separate observability session pattern This commit addresses valid concerns raised in PR review: 1. Fix class docstring showing old API (madhu-mohan-jaishankar) - Updated ObservabilityService class docstring to show correct API - Removed db parameter from examples (old: start_trace(db, name)) - Added clarification about write vs query methods - Examples now demonstrate correct usage with proper parameters 2. Fix context manager atomicity (msureshkumar88 issue #5) - Added optional obs_db parameter to add_event() method - Context managers now pass their session to add_event() in error paths - Ensures atomic commit of span updates and error events together - Prevents orphaned events when span update fails - Updated all three context managers: * trace_span() at line 644 * trace_tool_invocation() at line 757 * trace_a2a_request() at line 1112 3. Document connection pool sizing (msureshkumar88 issue #7) - Added Connection Pool Sizing section to AGENTS.md - Explains 4-6 sessions per traced request pattern - Documents default pool (200+10 = 210 connections) - Supports ~35 concurrent traced requests by default - Guidance for high-traffic deployments (>50 req/sec) - Note about SQLite 50-connection cap Implementation details: - add_event() now accepts optional obs_db: Optional[Session] = None - When obs_db is provided, uses caller's session (owned = False) - When obs_db is None, creates independent session (owned = True) - Only closes session if owned is True - All existing tests pass (87 observability service tests, 23 middleware tests) Tests: All 87 observability service tests passing All 23 observability middleware tests passing 100% differential coverage maintained Addresses review feedback from: - @madhu-mohan-jaishankar (approved with comments) - @msureshkumar88 (issues #5 and #7 from detailed review) Signed-off-by: Mohan Lakshmaiah <mohan.economist@gmail.com> * fix: address remaining review concerns with safety improvements This commit addresses all remaining valid reviewer concerns: 1. Document dead code guard design choice (madhu-mohan-jaishankar issue #1) - Added Note section to _get_or_create_observability_session() docstring - Explains tuple return pattern enables future session reuse optimization - Guards remain for forward compatibility (maintainer approved pattern) 2. Move session creation inside try blocks (msureshkumar88 issue #1) - Initialize obs_db = None and owned = False before try blocks - Create session inside try to prevent theoretical resource leak - Update finally blocks to check `if owned and obs_db is not None:` - Applied to all 8 write methods: * start_trace() (line 317) * end_trace() (line 385) * trace_span() context manager (line 619) * trace_tool_invocation() context manager (line 719) * add_event() (line 836) * record_token_usage() (line 926) * trace_a2a_request() context manager (line 1087) * record_metric() (line 1306) * delete_old_traces() (line 1718) 3. Add header sanitization (msureshkumar88 issue #10) - Added sanitize_header_for_storage() helper function - Removes control characters (prevents log injection) - Truncates to max_length (prevents DoS via large headers) - Applied to user_agent (max 500 chars) and http_url (max 2000 chars) - Defense-in-depth security improvement Implementation details: - sanitize_header_for_storage() removes non-printable chars except space/tab - Returns "unknown" for None/empty values - All 87 observability service tests passing - All 23 observability middleware tests passing Deferred to future PR (with justification): - Code duplication refactoring (issue #9): Too complex and risky for this PR. The three context managers share patterns but have significant domain-specific differences (attributes, yield signatures, sanitization). Refactoring would require extensive testing and carries risk of subtle bugs. Current code is correct and well-tested. Recommend separate focused PR for refactoring. Tests: All 110 observability tests passing (87 service + 23 middleware) 100% differential coverage maintained Addresses remaining review feedback from: - @madhu-mohan-jaishankar (issue #1 - dead code guards) - @msureshkumar88 (issues #1, #10 - resource leak, header sanitization) Signed-off-by: Mohan Lakshmaiah <mohan.economist@gmail.com> * fix: correct doctest examples in sanitize_header_for_storage The function removes control characters completely (doesn't replace with spaces) and truncates without adding ellipsis. Updated doctest examples to match actual behavior: - Control chars removed: 'Evil\x00\nInjection' -> 'EvilInjection' - Truncation verified by length check instead of expecting '...' - Added test for None input returning 'unknown' All doctests now passing. Signed-off-by: Mohan Lakshmaiah <mohan.economist@gmail.com> * Update secrets following rebase Signed-off-by: Brian Hussey <brian.hussey@ie.ibm.com> --------- Signed-off-by: Mohan Lakshmaiah <mohan.economist@gmail.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Signed-off-by: Brian Hussey <brian.hussey@ie.ibm.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Brian Hussey <brian.hussey@ie.ibm.com>
jonpspri
pushed a commit
that referenced
this pull request
Apr 10, 2026
…ons (#4050) * feat: implement separate observability session pattern (best-effort) Observability write operations now use independent database sessions that commit immediately on a best-effort basis, separate from main request transactions. This ensures observability data persists even when the main request fails, providing visibility into partial failures. Implementation details: - Observability write methods (start_trace, end_trace, start_span, end_span, add_event, record_metric, record_token_usage, delete_old_traces) now create their own independent sessions using SessionLocal() instead of accepting a db parameter from the caller - Context managers (trace_span, trace_tool_invocation, trace_a2a_request) create and manage a single session for the full span lifecycle - Query methods still accept db: Session parameter for RBAC/token scoping - ObservabilityMiddleware no longer creates or manages request.state.db - Each observability operation creates its own short-lived session that commits immediately in a try/except/finally block - Updated all callers across resource_service, prompt_service, tool_service, and instrumentation/sqlalchemy to use new API (removed db parameters) - Added comprehensive test coverage verifying separate session behavior - Fixed pre-existing bugs in add_event() and record_metric() where db.refresh() was called after commit failure Benefits: - Observability data persists independently of main request transaction success - Provides visibility into partial failures and error states - Follows pattern established in instrumentation/sqlalchemy.py (lines 58-87) - Reduces transaction contention between observability and business logic Trade-offs: - NOT atomic with main request transaction (intentional design choice) - Traces may show "in progress" or partial states for failed requests - Provides eventual consistency rather than strict consistency Documentation: - Added "Observability Transaction Behavior" section to AGENTS.md explaining the separate session pattern, implementation details, and security notes - Updated module docstrings with session management details Tests: 96 observability tests passing, full suite 16,673 tests passing Pylint: 10.00/10 rating maintained Closes #3883 Signed-off-by: Mohan Lakshmaiah <mohan.economist@gmail.com> * fix: add type annotations to observability context managers Add proper return type annotations to context manager methods: - trace_span() -> Generator[str, None, None] - trace_tool_invocation() -> Generator[Tuple[Optional[str], Dict[str, Any]], None, None] - trace_a2a_request() -> Generator[Tuple[Optional[str], Dict[str, Any]], None, None] These annotations improve type safety and make the return values explicit. The trace_span context manager yields just the span_id string, while trace_tool_invocation and trace_a2a_request yield a tuple of (span_id, result_dict) for capturing operation results. Tests: 96 observability tests passing Pylint: 10.00/10 rating maintained Signed-off-by: Mohan Lakshmaiah <mohan.economist@gmail.com> * test: Add coverage tests for observability session close exception handling Add 11 new test functions to achieve 100% coverage of exception handling paths in observability_service.py. These tests verify that all write methods and context managers handle session close failures gracefully: - test_start_trace_session_close_failure - test_end_trace_session_close_failure - test_start_span_session_close_failure - test_start_span_with_commit_true - test_end_span_session_close_failure - test_add_event_session_close_failure - test_record_metric_session_close_failure - test_delete_old_traces_session_close_failure - test_trace_span_context_manager_session_close_failure - test_trace_tool_invocation_context_manager_session_close_failure - test_trace_a2a_request_context_manager_session_close_failure Each test mocks the session close() method to raise an exception and verifies that the operation completes successfully despite the close failure. This covers the previously uncovered lines in finally blocks where session close exceptions are caught and logged. Resolves CI/CD coverage requirement for observability_service.py. Signed-off-by: Mohan Lakshmaiah <mohan.economist@gmail.com> * fix: complete separate session migration for delete_old_traces and record_transport_activity - Fix runtime TypeError in cleanup_old_traces router: delete_old_traces() no longer accepts db parameter (missed call site from #3883) - Remove unused db parameter from record_transport_activity() for API consistency with other write methods - Update docstring and doctest for delete_old_traces call in router - Update test calls to match new record_transport_activity signature - Add 3 tests for 100% differential coverage: error-path exception handlers in trace_tool_invocation, trace_a2a_request, and session close failure in record_token_usage Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * fix: address review feedback for separate observability session pattern This commit addresses valid concerns raised in PR review: 1. Fix class docstring showing old API (madhu-mohan-jaishankar) - Updated ObservabilityService class docstring to show correct API - Removed db parameter from examples (old: start_trace(db, name)) - Added clarification about write vs query methods - Examples now demonstrate correct usage with proper parameters 2. Fix context manager atomicity (msureshkumar88 issue #5) - Added optional obs_db parameter to add_event() method - Context managers now pass their session to add_event() in error paths - Ensures atomic commit of span updates and error events together - Prevents orphaned events when span update fails - Updated all three context managers: * trace_span() at line 644 * trace_tool_invocation() at line 757 * trace_a2a_request() at line 1112 3. Document connection pool sizing (msureshkumar88 issue #7) - Added Connection Pool Sizing section to AGENTS.md - Explains 4-6 sessions per traced request pattern - Documents default pool (200+10 = 210 connections) - Supports ~35 concurrent traced requests by default - Guidance for high-traffic deployments (>50 req/sec) - Note about SQLite 50-connection cap Implementation details: - add_event() now accepts optional obs_db: Optional[Session] = None - When obs_db is provided, uses caller's session (owned = False) - When obs_db is None, creates independent session (owned = True) - Only closes session if owned is True - All existing tests pass (87 observability service tests, 23 middleware tests) Tests: All 87 observability service tests passing All 23 observability middleware tests passing 100% differential coverage maintained Addresses review feedback from: - @madhu-mohan-jaishankar (approved with comments) - @msureshkumar88 (issues #5 and #7 from detailed review) Signed-off-by: Mohan Lakshmaiah <mohan.economist@gmail.com> * fix: address remaining review concerns with safety improvements This commit addresses all remaining valid reviewer concerns: 1. Document dead code guard design choice (madhu-mohan-jaishankar issue #1) - Added Note section to _get_or_create_observability_session() docstring - Explains tuple return pattern enables future session reuse optimization - Guards remain for forward compatibility (maintainer approved pattern) 2. Move session creation inside try blocks (msureshkumar88 issue #1) - Initialize obs_db = None and owned = False before try blocks - Create session inside try to prevent theoretical resource leak - Update finally blocks to check `if owned and obs_db is not None:` - Applied to all 8 write methods: * start_trace() (line 317) * end_trace() (line 385) * trace_span() context manager (line 619) * trace_tool_invocation() context manager (line 719) * add_event() (line 836) * record_token_usage() (line 926) * trace_a2a_request() context manager (line 1087) * record_metric() (line 1306) * delete_old_traces() (line 1718) 3. Add header sanitization (msureshkumar88 issue #10) - Added sanitize_header_for_storage() helper function - Removes control characters (prevents log injection) - Truncates to max_length (prevents DoS via large headers) - Applied to user_agent (max 500 chars) and http_url (max 2000 chars) - Defense-in-depth security improvement Implementation details: - sanitize_header_for_storage() removes non-printable chars except space/tab - Returns "unknown" for None/empty values - All 87 observability service tests passing - All 23 observability middleware tests passing Deferred to future PR (with justification): - Code duplication refactoring (issue #9): Too complex and risky for this PR. The three context managers share patterns but have significant domain-specific differences (attributes, yield signatures, sanitization). Refactoring would require extensive testing and carries risk of subtle bugs. Current code is correct and well-tested. Recommend separate focused PR for refactoring. Tests: All 110 observability tests passing (87 service + 23 middleware) 100% differential coverage maintained Addresses remaining review feedback from: - @madhu-mohan-jaishankar (issue #1 - dead code guards) - @msureshkumar88 (issues #1, #10 - resource leak, header sanitization) Signed-off-by: Mohan Lakshmaiah <mohan.economist@gmail.com> * fix: correct doctest examples in sanitize_header_for_storage The function removes control characters completely (doesn't replace with spaces) and truncates without adding ellipsis. Updated doctest examples to match actual behavior: - Control chars removed: 'Evil\x00\nInjection' -> 'EvilInjection' - Truncation verified by length check instead of expecting '...' - Added test for None input returning 'unknown' All doctests now passing. Signed-off-by: Mohan Lakshmaiah <mohan.economist@gmail.com> * Update secrets following rebase Signed-off-by: Brian Hussey <brian.hussey@ie.ibm.com> --------- Signed-off-by: Mohan Lakshmaiah <mohan.economist@gmail.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Signed-off-by: Brian Hussey <brian.hussey@ie.ibm.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Brian Hussey <brian.hussey@ie.ibm.com>
claudia-gray
pushed a commit
that referenced
this pull request
Apr 13, 2026
…ons (#4050) * feat: implement separate observability session pattern (best-effort) Observability write operations now use independent database sessions that commit immediately on a best-effort basis, separate from main request transactions. This ensures observability data persists even when the main request fails, providing visibility into partial failures. Implementation details: - Observability write methods (start_trace, end_trace, start_span, end_span, add_event, record_metric, record_token_usage, delete_old_traces) now create their own independent sessions using SessionLocal() instead of accepting a db parameter from the caller - Context managers (trace_span, trace_tool_invocation, trace_a2a_request) create and manage a single session for the full span lifecycle - Query methods still accept db: Session parameter for RBAC/token scoping - ObservabilityMiddleware no longer creates or manages request.state.db - Each observability operation creates its own short-lived session that commits immediately in a try/except/finally block - Updated all callers across resource_service, prompt_service, tool_service, and instrumentation/sqlalchemy to use new API (removed db parameters) - Added comprehensive test coverage verifying separate session behavior - Fixed pre-existing bugs in add_event() and record_metric() where db.refresh() was called after commit failure Benefits: - Observability data persists independently of main request transaction success - Provides visibility into partial failures and error states - Follows pattern established in instrumentation/sqlalchemy.py (lines 58-87) - Reduces transaction contention between observability and business logic Trade-offs: - NOT atomic with main request transaction (intentional design choice) - Traces may show "in progress" or partial states for failed requests - Provides eventual consistency rather than strict consistency Documentation: - Added "Observability Transaction Behavior" section to AGENTS.md explaining the separate session pattern, implementation details, and security notes - Updated module docstrings with session management details Tests: 96 observability tests passing, full suite 16,673 tests passing Pylint: 10.00/10 rating maintained Closes #3883 Signed-off-by: Mohan Lakshmaiah <mohan.economist@gmail.com> * fix: add type annotations to observability context managers Add proper return type annotations to context manager methods: - trace_span() -> Generator[str, None, None] - trace_tool_invocation() -> Generator[Tuple[Optional[str], Dict[str, Any]], None, None] - trace_a2a_request() -> Generator[Tuple[Optional[str], Dict[str, Any]], None, None] These annotations improve type safety and make the return values explicit. The trace_span context manager yields just the span_id string, while trace_tool_invocation and trace_a2a_request yield a tuple of (span_id, result_dict) for capturing operation results. Tests: 96 observability tests passing Pylint: 10.00/10 rating maintained Signed-off-by: Mohan Lakshmaiah <mohan.economist@gmail.com> * test: Add coverage tests for observability session close exception handling Add 11 new test functions to achieve 100% coverage of exception handling paths in observability_service.py. These tests verify that all write methods and context managers handle session close failures gracefully: - test_start_trace_session_close_failure - test_end_trace_session_close_failure - test_start_span_session_close_failure - test_start_span_with_commit_true - test_end_span_session_close_failure - test_add_event_session_close_failure - test_record_metric_session_close_failure - test_delete_old_traces_session_close_failure - test_trace_span_context_manager_session_close_failure - test_trace_tool_invocation_context_manager_session_close_failure - test_trace_a2a_request_context_manager_session_close_failure Each test mocks the session close() method to raise an exception and verifies that the operation completes successfully despite the close failure. This covers the previously uncovered lines in finally blocks where session close exceptions are caught and logged. Resolves CI/CD coverage requirement for observability_service.py. Signed-off-by: Mohan Lakshmaiah <mohan.economist@gmail.com> * fix: complete separate session migration for delete_old_traces and record_transport_activity - Fix runtime TypeError in cleanup_old_traces router: delete_old_traces() no longer accepts db parameter (missed call site from #3883) - Remove unused db parameter from record_transport_activity() for API consistency with other write methods - Update docstring and doctest for delete_old_traces call in router - Update test calls to match new record_transport_activity signature - Add 3 tests for 100% differential coverage: error-path exception handlers in trace_tool_invocation, trace_a2a_request, and session close failure in record_token_usage Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> * fix: address review feedback for separate observability session pattern This commit addresses valid concerns raised in PR review: 1. Fix class docstring showing old API (madhu-mohan-jaishankar) - Updated ObservabilityService class docstring to show correct API - Removed db parameter from examples (old: start_trace(db, name)) - Added clarification about write vs query methods - Examples now demonstrate correct usage with proper parameters 2. Fix context manager atomicity (msureshkumar88 issue #5) - Added optional obs_db parameter to add_event() method - Context managers now pass their session to add_event() in error paths - Ensures atomic commit of span updates and error events together - Prevents orphaned events when span update fails - Updated all three context managers: * trace_span() at line 644 * trace_tool_invocation() at line 757 * trace_a2a_request() at line 1112 3. Document connection pool sizing (msureshkumar88 issue #7) - Added Connection Pool Sizing section to AGENTS.md - Explains 4-6 sessions per traced request pattern - Documents default pool (200+10 = 210 connections) - Supports ~35 concurrent traced requests by default - Guidance for high-traffic deployments (>50 req/sec) - Note about SQLite 50-connection cap Implementation details: - add_event() now accepts optional obs_db: Optional[Session] = None - When obs_db is provided, uses caller's session (owned = False) - When obs_db is None, creates independent session (owned = True) - Only closes session if owned is True - All existing tests pass (87 observability service tests, 23 middleware tests) Tests: All 87 observability service tests passing All 23 observability middleware tests passing 100% differential coverage maintained Addresses review feedback from: - @madhu-mohan-jaishankar (approved with comments) - @msureshkumar88 (issues #5 and #7 from detailed review) Signed-off-by: Mohan Lakshmaiah <mohan.economist@gmail.com> * fix: address remaining review concerns with safety improvements This commit addresses all remaining valid reviewer concerns: 1. Document dead code guard design choice (madhu-mohan-jaishankar issue #1) - Added Note section to _get_or_create_observability_session() docstring - Explains tuple return pattern enables future session reuse optimization - Guards remain for forward compatibility (maintainer approved pattern) 2. Move session creation inside try blocks (msureshkumar88 issue #1) - Initialize obs_db = None and owned = False before try blocks - Create session inside try to prevent theoretical resource leak - Update finally blocks to check `if owned and obs_db is not None:` - Applied to all 8 write methods: * start_trace() (line 317) * end_trace() (line 385) * trace_span() context manager (line 619) * trace_tool_invocation() context manager (line 719) * add_event() (line 836) * record_token_usage() (line 926) * trace_a2a_request() context manager (line 1087) * record_metric() (line 1306) * delete_old_traces() (line 1718) 3. Add header sanitization (msureshkumar88 issue #10) - Added sanitize_header_for_storage() helper function - Removes control characters (prevents log injection) - Truncates to max_length (prevents DoS via large headers) - Applied to user_agent (max 500 chars) and http_url (max 2000 chars) - Defense-in-depth security improvement Implementation details: - sanitize_header_for_storage() removes non-printable chars except space/tab - Returns "unknown" for None/empty values - All 87 observability service tests passing - All 23 observability middleware tests passing Deferred to future PR (with justification): - Code duplication refactoring (issue #9): Too complex and risky for this PR. The three context managers share patterns but have significant domain-specific differences (attributes, yield signatures, sanitization). Refactoring would require extensive testing and carries risk of subtle bugs. Current code is correct and well-tested. Recommend separate focused PR for refactoring. Tests: All 110 observability tests passing (87 service + 23 middleware) 100% differential coverage maintained Addresses remaining review feedback from: - @madhu-mohan-jaishankar (issue #1 - dead code guards) - @msureshkumar88 (issues #1, #10 - resource leak, header sanitization) Signed-off-by: Mohan Lakshmaiah <mohan.economist@gmail.com> * fix: correct doctest examples in sanitize_header_for_storage The function removes control characters completely (doesn't replace with spaces) and truncates without adding ellipsis. Updated doctest examples to match actual behavior: - Control chars removed: 'Evil\x00\nInjection' -> 'EvilInjection' - Truncation verified by length check instead of expecting '...' - Added test for None input returning 'unknown' All doctests now passing. Signed-off-by: Mohan Lakshmaiah <mohan.economist@gmail.com> * Update secrets following rebase Signed-off-by: Brian Hussey <brian.hussey@ie.ibm.com> --------- Signed-off-by: Mohan Lakshmaiah <mohan.economist@gmail.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com> Signed-off-by: Brian Hussey <brian.hussey@ie.ibm.com> Co-authored-by: Mihai Criveti <crivetimihai@gmail.com> Co-authored-by: Brian Hussey <brian.hussey@ie.ibm.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>
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 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
2 participants
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.