Add postgresql to container build - #5
Merged
Merged
Conversation
Merged
vk-playground
pushed a commit
to vk-playground/mcp-context-forge
that referenced
this pull request
Sep 14, 2025
Add postgresql to container build 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
Add postgresql to container build
vk-playground
pushed a commit
to vk-playground/mcp-context-forge
that referenced
this pull request
Sep 16, 2025
Add postgresql to container build Signed-off-by: Vicky Kuo <vicky.kuo@ibm.com>
This was referenced Oct 11, 2025
hughhennelly
added a commit
to hughhennelly/mcp-context-forge
that referenced
this pull request
Feb 12, 2026
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 feedback from @jonpspri: Problem: PR description claimed 'Full audit trail of all access decisions' but _log_decision() only logged to Python logger with TODO comment. Solution: - Updated docstring to clarify DB audit logging is Phase 2 - Changed logging level from INFO to DEBUG (reduce production noise) - AccessDecisionLog table is created and ready for Phase 2 - Honest about current state vs future implementation Result: - Clear documentation that DB audit is Phase 2 scaffolding - Application logging still captures all decisions - Table structure ready for Phase 2 implementation - No misleading claims in PR description Related: PR IBM#2682 Phase 1 Code Review Item IBM#5 Signed-off-by: yiannis2804 <yiannis2804@gmail.com>
crivetimihai
pushed a commit
that referenced
this pull request
Feb 24, 2026
Address code review feedback from @jonpspri: Problem: PR description claimed 'Full audit trail of all access decisions' but _log_decision() only logged to Python logger with TODO comment. Solution: - Updated docstring to clarify DB audit logging is Phase 2 - Changed logging level from INFO to DEBUG (reduce production noise) - AccessDecisionLog table is created and ready for Phase 2 - Honest about current state vs future implementation Result: - Clear documentation that DB audit is Phase 2 scaffolding - Application logging still captures all decisions - Table structure ready for Phase 2 implementation - No misleading claims in PR description Related: PR #2682 Phase 1 Code Review Item #5 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>
12 tasks
10 tasks
jonpspri
added a commit
that referenced
this pull request
Jun 25, 2026
Adds A2AAgentService.dispatch_a2a_jsonrpc_streaming — the SEPARATE codepath T12 uses for streaming methods (SendStreamingMessage, SubscribeToTask). Per plan T5 + D10 + D15 + Oracle v2 #5 + v3 #5, this helper streams the upstream response through async with client.stream() rather than buffering it through the unary invoke_agent pipeline. Key behaviors: - REAL SSE parser (Oracle v2 #5 fix) — accumulates data: lines until a blank-line delimiter, then parses the joined payload as JSON and yields one dict per upstream event. Other SSE field lines (id:, event:, retry:) are ignored per A2A 1.0.0 spec F8. - Malformed JSON in a data: payload yields ONE INVALID_AGENT_RESPONSE chunk and the parser CONTINUES, not exits. - Upstream HTTP error (4xx/5xx) before stream begins yields ONE INTERNAL_ERROR chunk + exits cleanly. - httpx.HTTPError (network failure, connection refused, etc.) yields ONE INTERNAL_ERROR chunk + exits. - Generator cancellation: async with client.stream(...) automatically closes the upstream connection. Envelope handling (mirrors T4 unary semantics): - Validates jsonrpc=='2.0', method non-empty string, params dict-or-null - Maps legacy v0.3 method aliases to v1 names via LEGACY_V03_METHOD_MAP (tasks/list is NEW in v1.0, NOT mapped per Oracle v3 #22). - Hop-count loop guard mirrors invoke_agent (a2a_service.py:2587-2595). Outbound headers built carefully: - Accept: text/event-stream - Content-Type: application/json - A2A-Version: agent.protocol_version (Oracle v3 #21 — NOT hardcoded) - X-Contextforge-UAID-Hop: hop_count + 1 (federation loop guard) - Authorization: Bearer <jwt> only when bearer_token is truthy - request_headers passed through (caller filters sensitive headers) Signature matches T12's exact call shape (Oracle v3 #2 + v4): (db, agent, body, *, bearer_token, hop_count, request_headers). Uses request_headers kwarg name (NOT forward_headers from earlier revisions). Tests (TestDispatchA2AJsonrpcStreaming, 16 cases): - Multiple SSE chunks streamed → multiple yielded dicts (T5 acceptance a) - Multi-line data: accumulation works (T5 acceptance b) - Blank-line-only frames ignored (T5 acceptance c) - Ignored SSE fields (id:, event:, retry:) skipped per F8 - Malformed JSON yields INVALID_AGENT_RESPONSE chunk + continues (T5 acceptance f) - Upstream HTTP error yields one INTERNAL_ERROR chunk + exits (T5 acceptance e) - Envelope validation: jsonrpc/method/params errors yield error chunks - Hop count limit reached yields error + skips upstream call - Legacy alias mapping verified for upstream body - tasks/list NOT mapped (Oracle v3 #22) - Outbound headers: Accept, Content-Type, A2A-Version from agent, X-Contextforge-UAID-Hop incremented, Authorization with bearer, request_headers passthrough - No bearer → no Authorization header (defense-in-depth) - httpx.HTTPError yields INTERNAL_ERROR chunk 119/119 native tests pass (15 T1 schemas + 10 T3 + 14 T2 + 18 T6 + 34 T7 + 23 T4 + 16 T5 = 130 — actually 119 because some classes share tests). Wave 1 complete: T1, T2, T3, T4, T5, T6, T7 — all 7 foundation todos landed with verified-clean tests + ruff + black + IBM Detect Secrets. Signed-off-by: Jonathan Springer <jps@s390x.com>
jonpspri
added a commit
that referenced
this pull request
Jun 25, 2026
…ing (T12 + T14)
Adds the per-agent JSON-RPC dispatch route and SSE re-wrap helper to
mcpgateway/main.py:
POST /a2a/{agent_name}
T12 — dispatch_a2a_agent handler:
- NO @require_permission decorator (Oracle v2 #1 — body-dependent RBAC
requires per-method check).
- NO Body(...) parameter (D17 — preserves raw body for -32700 ParseError).
- 9-step strict-order handler flow mirroring the verified /invoke
pipeline at main.py:5040-5137:
1. get_rpc_filter_context + admin/public-only token reshape.
2. resolve_agent_for_dispatch (T3) → 404 on visibility miss /
v-server-foreign / unknown (D14).
3. uaid_utils.read_hop_count + bearer_token + content_type +
_filter_sensitive_headers (Oracle v3 #3: real code uses
uaid_utils.read_hop_count, NOT the previous plan's fictitious
X-Forwarded-A2A-Hop).
4. Parse body. JSONDecodeError → 200 + -32700. Non-object →
200 + -32600 (Oracle v2 #7 isinstance(dict) guard).
5. validate_a2a_version (T7, method-aware) → 200 + -32009 on
VersionNotSupportedError.
6. Method-dependent RBAC with verified check_permission signature
(user_email=, NOT user= — Oracle v3 #1). Passes token_teams so
permission_service.py:126-130 admin-bypass-suppression fires.
7. GetExtendedAgentCard / agent/getAuthenticatedExtendedCard:
a2a.read permission check → 403 if denied. Capability gate via
agent.capabilities.extendedAgentCard → 200 + -32007 if False.
NEVER forwards upstream (D18). Synthesizes card directly via T2
with authenticated user_email + token_teams (NOT None/[]).
8. Streaming methods (_A2A_STREAMING_METHODS frozenset includes
SendStreamingMessage, SubscribeToTask, message/stream,
tasks/resubscribe v0.3 alias): T5 streaming dispatch + T14
SSE re-wrap via StreamingResponse. NO await on T5 — it returns
an async generator (Oracle v5 HIGH fix).
9. Else: T4 unary dispatch. Success dict → 200 + JSON-RPC result
envelope. Error tuple (code, msg, data) → 200 + make_jsonrpc_error
envelope (D6).
T14 — _sse_format helper:
- Re-wraps T5's parsed-dict yields as one downstream
'data: {json}\n\n' event per upstream chunk.
- Compact JSON via separators=(',', ':') minimizes wire bytes.
- No double-encoding (T5 has already stripped upstream data: framing
per Oracle re-review #5 pairing fix).
CRITICAL ROUTE ORDERING FIX:
- POST /a2a/{agent_name} is registered AFTER POST /a2a/invoke so the
literal /a2a/invoke path resolves to the legacy ID-based handler
and is NOT shadowed by the catch-all. Verified via app.routes
introspection and by the 11 previously-failing TestA2AInvokeBodyEndpoint
unit tests now passing again. T13 (next Wave 3 todo) writes the
explicit route-ordering regression test.
Imports added to main.py:
- get_permission_service from mcpgateway.middleware.rbac
- a2a_service constants/helpers: AUTHENTICATED_EXTENDED_CARD_NOT_CONFIGURED,
INVALID_REQUEST, PARSE_ERROR, VERSION_NOT_SUPPORTED, VersionNotSupportedError,
make_jsonrpc_error, validate_a2a_version (all module-level from Wave 1
T6 + T7).
Tests: tests/integration/test_a2a_native_routes.py adds:
- TestPerAgentDispatchEndpoint: 11 tests covering all 9 QA scenarios
from the plan plus 503 (a2a_service None) and tuple-error-envelope
path.
- TestSseFormatHelper: 3 unit tests for the SSE re-wrap helper
verifying one-event-per-chunk, compact JSON separators, and no
double-encoding.
- CSRF middleware bypassed via 'Authorization: Bearer fake-test-token'
header (csrf_middleware.py:113-116 skips Bearer-authenticated
requests since they are not browser-driven).
- get_rpc_filter_context patched at module-attribute level (TestClient
does not populate request.state._jwt_verified_payload).
- Cache-Control assertion accepts both 'no-cache' (handler value) and
'no-store' (security middleware override).
Verified:
- 23/23 integration tests in test_a2a_native_routes.py PASS (with
--with-integration flag).
- 11 previously-failing tests/unit/mcpgateway/test_main.py::TestA2AInvokeBodyEndpoint
tests now PASS again (route ordering fix verified).
- Full make test (unit): 18702 passed, 120 skipped, 2 xfailed, 0 failed.
- Evidence: .omo/evidence/task-12-a2a-native-passthrough.txt.
Wave 2 compliance impact: T12 + T14 satisfy 22 of the 25 T10 BLOCK
rows from .omo/evidence/c4-audit-checklist.md Sections 2-8 (envelope
validation, method catalog, error codes including -32006/-32007/-32009,
SSE shape, A2A-Version negotiation, v0.3 alias mapping, transport-level
401/403 RBAC denial). The remaining 3 Section-8 BLOCK rows (team-scoped
404, with-read 200, without-read 403) require RBAC role fixtures
deferred to Wave 7 T28-B per the audit.
Refs: .omo/plans/a2a-native-passthrough.md T12 + T14
Next: T13 (route-ordering regression test), T15 (proxy compliance smoke).
Signed-off-by: Jonathan Springer <jps@s390x.com>
jonpspri
added a commit
that referenced
this pull request
Jun 25, 2026
…Config (T29) Plan T29 (Wave 7) — wire up the two gateway placeholder targets to the native A2A passthrough that landed in Wave 3 + Wave 4. The matrix tests in v1_0_0/ now run end-to-end against the live gateway-proxy and gateway-virtual surfaces instead of xfailing. Per-target shape mirrors targets/reference.py:55-65 EXACTLY (Oracle v3 #7 + v4 #5 — the prior plan invented non-existent attributes; the verified attributes from the constructors at gateway_proxy.py:44-47 and gateway_virtual.py:32-36 are _base_url, _auth_token, _agent_name, and additionally _server_id for the v-server target): tests/live_gateway/a2a_compliance/targets/gateway_proxy.py: - Replaced the placeholder NotImplementedError in _open_client with the canonical asynccontextmanager body: httpx.AsyncClient with base_url + Authorization Bearer header, ClientConfig wrapping that client, ClientFactory.create_from_url against '{base_url}/a2a/{agent_name}' so the SDK fetches the synthesized card at the T11 well-known endpoint and picks the JSON-RPC transport per its protocolBinding (D8). - Module docstring updated to lead with the T29 wiring story instead of A2A-GAP-001 placeholder framing. tests/live_gateway/a2a_compliance/targets/gateway_virtual.py: - Same canonical body as gateway_proxy, with the URL becoming '{base_url}/servers/{server_id}/a2a/{agent_name}' so the T16 path-rewrite middleware populates request.scope['a2a_server_id'] before the per-agent handler runs. The three-level conjunctive v-server access (Amendment B) gates the synthesizer there. tests/live_gateway/a2a_compliance/conftest.py: - _build_target replaces the 'http://placeholder' / 'placeholder' literals with real fixture lookups for the gateway targets: gateway_base_url + auth_token + registered_agent_name (with a side-effecting registered_agent_id lookup to trigger gateway probe + agent registration), plus server_id for gateway_virtual. Verification: uv run python -c 'from tests.live_gateway.a2a_compliance.targets...' -> imports OK, both classes resolve their name + transports cleanly (T30 follow-up — close A2A-GAP-001 + delete the matrix-wide xfail hook in the same conftest.py + remove the ad-hoc scripts/qa/a2a_*_smoke.py that the harness now subsumes — lands as the next commit.) Plan reference: .omo/plans/a2a-native-passthrough.md T29 (Wave 7). Signed-off-by: Jonathan Springer <jps@s390x.com>
jonpspri
added a commit
that referenced
this pull request
Jun 26, 2026
Adds A2AAgentService.dispatch_a2a_jsonrpc_streaming — the SEPARATE codepath T12 uses for streaming methods (SendStreamingMessage, SubscribeToTask). Per plan T5 + D10 + D15 + Oracle v2 #5 + v3 #5, this helper streams the upstream response through async with client.stream() rather than buffering it through the unary invoke_agent pipeline. Key behaviors: - REAL SSE parser (Oracle v2 #5 fix) — accumulates data: lines until a blank-line delimiter, then parses the joined payload as JSON and yields one dict per upstream event. Other SSE field lines (id:, event:, retry:) are ignored per A2A 1.0.0 spec F8. - Malformed JSON in a data: payload yields ONE INVALID_AGENT_RESPONSE chunk and the parser CONTINUES, not exits. - Upstream HTTP error (4xx/5xx) before stream begins yields ONE INTERNAL_ERROR chunk + exits cleanly. - httpx.HTTPError (network failure, connection refused, etc.) yields ONE INTERNAL_ERROR chunk + exits. - Generator cancellation: async with client.stream(...) automatically closes the upstream connection. Envelope handling (mirrors T4 unary semantics): - Validates jsonrpc=='2.0', method non-empty string, params dict-or-null - Maps legacy v0.3 method aliases to v1 names via LEGACY_V03_METHOD_MAP (tasks/list is NEW in v1.0, NOT mapped per Oracle v3 #22). - Hop-count loop guard mirrors invoke_agent (a2a_service.py:2587-2595). Outbound headers built carefully: - Accept: text/event-stream - Content-Type: application/json - A2A-Version: agent.protocol_version (Oracle v3 #21 — NOT hardcoded) - X-Contextforge-UAID-Hop: hop_count + 1 (federation loop guard) - Authorization: Bearer <jwt> only when bearer_token is truthy - request_headers passed through (caller filters sensitive headers) Signature matches T12's exact call shape (Oracle v3 #2 + v4): (db, agent, body, *, bearer_token, hop_count, request_headers). Uses request_headers kwarg name (NOT forward_headers from earlier revisions). Tests (TestDispatchA2AJsonrpcStreaming, 16 cases): - Multiple SSE chunks streamed → multiple yielded dicts (T5 acceptance a) - Multi-line data: accumulation works (T5 acceptance b) - Blank-line-only frames ignored (T5 acceptance c) - Ignored SSE fields (id:, event:, retry:) skipped per F8 - Malformed JSON yields INVALID_AGENT_RESPONSE chunk + continues (T5 acceptance f) - Upstream HTTP error yields one INTERNAL_ERROR chunk + exits (T5 acceptance e) - Envelope validation: jsonrpc/method/params errors yield error chunks - Hop count limit reached yields error + skips upstream call - Legacy alias mapping verified for upstream body - tasks/list NOT mapped (Oracle v3 #22) - Outbound headers: Accept, Content-Type, A2A-Version from agent, X-Contextforge-UAID-Hop incremented, Authorization with bearer, request_headers passthrough - No bearer → no Authorization header (defense-in-depth) - httpx.HTTPError yields INTERNAL_ERROR chunk 119/119 native tests pass (15 T1 schemas + 10 T3 + 14 T2 + 18 T6 + 34 T7 + 23 T4 + 16 T5 = 130 — actually 119 because some classes share tests). Wave 1 complete: T1, T2, T3, T4, T5, T6, T7 — all 7 foundation todos landed with verified-clean tests + ruff + black + IBM Detect Secrets. Signed-off-by: Jonathan Springer <jps@s390x.com>
jonpspri
added a commit
that referenced
this pull request
Jun 26, 2026
…ing (T12 + T14)
Adds the per-agent JSON-RPC dispatch route and SSE re-wrap helper to
mcpgateway/main.py:
POST /a2a/{agent_name}
T12 — dispatch_a2a_agent handler:
- NO @require_permission decorator (Oracle v2 #1 — body-dependent RBAC
requires per-method check).
- NO Body(...) parameter (D17 — preserves raw body for -32700 ParseError).
- 9-step strict-order handler flow mirroring the verified /invoke
pipeline at main.py:5040-5137:
1. get_rpc_filter_context + admin/public-only token reshape.
2. resolve_agent_for_dispatch (T3) → 404 on visibility miss /
v-server-foreign / unknown (D14).
3. uaid_utils.read_hop_count + bearer_token + content_type +
_filter_sensitive_headers (Oracle v3 #3: real code uses
uaid_utils.read_hop_count, NOT the previous plan's fictitious
X-Forwarded-A2A-Hop).
4. Parse body. JSONDecodeError → 200 + -32700. Non-object →
200 + -32600 (Oracle v2 #7 isinstance(dict) guard).
5. validate_a2a_version (T7, method-aware) → 200 + -32009 on
VersionNotSupportedError.
6. Method-dependent RBAC with verified check_permission signature
(user_email=, NOT user= — Oracle v3 #1). Passes token_teams so
permission_service.py:126-130 admin-bypass-suppression fires.
7. GetExtendedAgentCard / agent/getAuthenticatedExtendedCard:
a2a.read permission check → 403 if denied. Capability gate via
agent.capabilities.extendedAgentCard → 200 + -32007 if False.
NEVER forwards upstream (D18). Synthesizes card directly via T2
with authenticated user_email + token_teams (NOT None/[]).
8. Streaming methods (_A2A_STREAMING_METHODS frozenset includes
SendStreamingMessage, SubscribeToTask, message/stream,
tasks/resubscribe v0.3 alias): T5 streaming dispatch + T14
SSE re-wrap via StreamingResponse. NO await on T5 — it returns
an async generator (Oracle v5 HIGH fix).
9. Else: T4 unary dispatch. Success dict → 200 + JSON-RPC result
envelope. Error tuple (code, msg, data) → 200 + make_jsonrpc_error
envelope (D6).
T14 — _sse_format helper:
- Re-wraps T5's parsed-dict yields as one downstream
'data: {json}\n\n' event per upstream chunk.
- Compact JSON via separators=(',', ':') minimizes wire bytes.
- No double-encoding (T5 has already stripped upstream data: framing
per Oracle re-review #5 pairing fix).
CRITICAL ROUTE ORDERING FIX:
- POST /a2a/{agent_name} is registered AFTER POST /a2a/invoke so the
literal /a2a/invoke path resolves to the legacy ID-based handler
and is NOT shadowed by the catch-all. Verified via app.routes
introspection and by the 11 previously-failing TestA2AInvokeBodyEndpoint
unit tests now passing again. T13 (next Wave 3 todo) writes the
explicit route-ordering regression test.
Imports added to main.py:
- get_permission_service from mcpgateway.middleware.rbac
- a2a_service constants/helpers: AUTHENTICATED_EXTENDED_CARD_NOT_CONFIGURED,
INVALID_REQUEST, PARSE_ERROR, VERSION_NOT_SUPPORTED, VersionNotSupportedError,
make_jsonrpc_error, validate_a2a_version (all module-level from Wave 1
T6 + T7).
Tests: tests/integration/test_a2a_native_routes.py adds:
- TestPerAgentDispatchEndpoint: 11 tests covering all 9 QA scenarios
from the plan plus 503 (a2a_service None) and tuple-error-envelope
path.
- TestSseFormatHelper: 3 unit tests for the SSE re-wrap helper
verifying one-event-per-chunk, compact JSON separators, and no
double-encoding.
- CSRF middleware bypassed via 'Authorization: Bearer fake-test-token'
header (csrf_middleware.py:113-116 skips Bearer-authenticated
requests since they are not browser-driven).
- get_rpc_filter_context patched at module-attribute level (TestClient
does not populate request.state._jwt_verified_payload).
- Cache-Control assertion accepts both 'no-cache' (handler value) and
'no-store' (security middleware override).
Verified:
- 23/23 integration tests in test_a2a_native_routes.py PASS (with
--with-integration flag).
- 11 previously-failing tests/unit/mcpgateway/test_main.py::TestA2AInvokeBodyEndpoint
tests now PASS again (route ordering fix verified).
- Full make test (unit): 18702 passed, 120 skipped, 2 xfailed, 0 failed.
- Evidence: .omo/evidence/task-12-a2a-native-passthrough.txt.
Wave 2 compliance impact: T12 + T14 satisfy 22 of the 25 T10 BLOCK
rows from .omo/evidence/c4-audit-checklist.md Sections 2-8 (envelope
validation, method catalog, error codes including -32006/-32007/-32009,
SSE shape, A2A-Version negotiation, v0.3 alias mapping, transport-level
401/403 RBAC denial). The remaining 3 Section-8 BLOCK rows (team-scoped
404, with-read 200, without-read 403) require RBAC role fixtures
deferred to Wave 7 T28-B per the audit.
Refs: .omo/plans/a2a-native-passthrough.md T12 + T14
Next: T13 (route-ordering regression test), T15 (proxy compliance smoke).
Signed-off-by: Jonathan Springer <jps@s390x.com>
8 tasks
Closed
16 tasks
bogdanmariusc10
pushed a commit
that referenced
this pull request
Jul 15, 2026
…n error diagnostics Address all blocking issues and warnings from code review: 1. **Type Safety Enhancement** (Issue #5) - Improve type: ignore comment with detailed rationale - Explain why type checker cannot infer concrete Exception after unwrapping - Lines: upstream_session_registry.py:387-391 2. **Logging Diagnostics Test Coverage** (Issue #2) - Add test_logger_error_call_with_exc_info: verify logger.error called with exc_info - Add test_structured_logger_metadata_payload: verify structured logger metadata - Tests validate both standard logging (with traceback) and structured logging paths - Lines: test_upstream_session_error_categories.py:459-543 3. **Cross-Layer Consistency Regression Test** (Issue #3) - Add test_cross_layer_error_message_consistency - Verify registry RuntimeError surfaces categorized text through consuming layer - Ensures fix remains effective across error propagation chain - Lines: test_upstream_session_error_categories.py:547-599 4. **Feature Documentation** (Issue #1 - Blocking) - Add comprehensive "Upstream Session Error Diagnostics" section to observability-otel.md - Document all 13 error categories with descriptions and common causes - Include error message format examples - Provide structured logging metadata schema - Add monitoring/alerting examples (Prometheus, Datadog, Splunk) - Explain ExceptionGroup unwrapping behavior - Lines: observability-otel.md:651-756 All tests pass (18/18). Code review findings fully addressed. Related: #5608 Signed-off-by: Bogdan-Marius-Catanus <bogdan-marius.catanus@ibm.com>
bogdanmariusc10
pushed a commit
that referenced
this pull request
Jul 27, 2026
…n error diagnostics Address all blocking issues and warnings from code review: 1. **Type Safety Enhancement** (Issue #5) - Improve type: ignore comment with detailed rationale - Explain why type checker cannot infer concrete Exception after unwrapping - Lines: upstream_session_registry.py:387-391 2. **Logging Diagnostics Test Coverage** (Issue #2) - Add test_logger_error_call_with_exc_info: verify logger.error called with exc_info - Add test_structured_logger_metadata_payload: verify structured logger metadata - Tests validate both standard logging (with traceback) and structured logging paths - Lines: test_upstream_session_error_categories.py:459-543 3. **Cross-Layer Consistency Regression Test** (Issue #3) - Add test_cross_layer_error_message_consistency - Verify registry RuntimeError surfaces categorized text through consuming layer - Ensures fix remains effective across error propagation chain - Lines: test_upstream_session_error_categories.py:547-599 4. **Feature Documentation** (Issue #1 - Blocking) - Add comprehensive "Upstream Session Error Diagnostics" section to observability-otel.md - Document all 13 error categories with descriptions and common causes - Include error message format examples - Provide structured logging metadata schema - Add monitoring/alerting examples (Prometheus, Datadog, Splunk) - Explain ExceptionGroup unwrapping behavior - Lines: observability-otel.md:651-756 All tests pass (18/18). Code review findings fully addressed. Related: #5608 Signed-off-by: Bogdan-Marius-Catanus <bogdan-marius.catanus@ibm.com>
19 tasks
bogdanmariusc10
pushed a commit
that referenced
this pull request
Jul 28, 2026
…n error diagnostics Address all blocking issues and warnings from code review: 1. **Type Safety Enhancement** (Issue #5) - Improve type: ignore comment with detailed rationale - Explain why type checker cannot infer concrete Exception after unwrapping - Lines: upstream_session_registry.py:387-391 2. **Logging Diagnostics Test Coverage** (Issue #2) - Add test_logger_error_call_with_exc_info: verify logger.error called with exc_info - Add test_structured_logger_metadata_payload: verify structured logger metadata - Tests validate both standard logging (with traceback) and structured logging paths - Lines: test_upstream_session_error_categories.py:459-543 3. **Cross-Layer Consistency Regression Test** (Issue #3) - Add test_cross_layer_error_message_consistency - Verify registry RuntimeError surfaces categorized text through consuming layer - Ensures fix remains effective across error propagation chain - Lines: test_upstream_session_error_categories.py:547-599 4. **Feature Documentation** (Issue #1 - Blocking) - Add comprehensive "Upstream Session Error Diagnostics" section to observability-otel.md - Document all 13 error categories with descriptions and common causes - Include error message format examples - Provide structured logging metadata schema - Add monitoring/alerting examples (Prometheus, Datadog, Splunk) - Explain ExceptionGroup unwrapping behavior - Lines: observability-otel.md:651-756 All tests pass (18/18). Code review findings fully addressed. Related: #5608 Signed-off-by: Bogdan-Marius-Catanus <bogdan-marius.catanus@ibm.com>
bogdanmariusc10
pushed a commit
that referenced
this pull request
Jul 29, 2026
…n error diagnostics Address all blocking issues and warnings from code review: 1. **Type Safety Enhancement** (Issue #5) - Improve type: ignore comment with detailed rationale - Explain why type checker cannot infer concrete Exception after unwrapping - Lines: upstream_session_registry.py:387-391 2. **Logging Diagnostics Test Coverage** (Issue #2) - Add test_logger_error_call_with_exc_info: verify logger.error called with exc_info - Add test_structured_logger_metadata_payload: verify structured logger metadata - Tests validate both standard logging (with traceback) and structured logging paths - Lines: test_upstream_session_error_categories.py:459-543 3. **Cross-Layer Consistency Regression Test** (Issue #3) - Add test_cross_layer_error_message_consistency - Verify registry RuntimeError surfaces categorized text through consuming layer - Ensures fix remains effective across error propagation chain - Lines: test_upstream_session_error_categories.py:547-599 4. **Feature Documentation** (Issue #1 - Blocking) - Add comprehensive "Upstream Session Error Diagnostics" section to observability-otel.md - Document all 13 error categories with descriptions and common causes - Include error message format examples - Provide structured logging metadata schema - Add monitoring/alerting examples (Prometheus, Datadog, Splunk) - Explain ExceptionGroup unwrapping behavior - Lines: observability-otel.md:651-756 All tests pass (18/18). Code review findings fully addressed. Related: #5608 Signed-off-by: Bogdan-Marius-Catanus <bogdan-marius.catanus@ibm.com>
ja8zyjits
pushed a commit
that referenced
this pull request
Jul 30, 2026
…n error diagnostics Address all blocking issues and warnings from code review: 1. **Type Safety Enhancement** (Issue #5) - Improve type: ignore comment with detailed rationale - Explain why type checker cannot infer concrete Exception after unwrapping - Lines: upstream_session_registry.py:387-391 2. **Logging Diagnostics Test Coverage** (Issue #2) - Add test_logger_error_call_with_exc_info: verify logger.error called with exc_info - Add test_structured_logger_metadata_payload: verify structured logger metadata - Tests validate both standard logging (with traceback) and structured logging paths - Lines: test_upstream_session_error_categories.py:459-543 3. **Cross-Layer Consistency Regression Test** (Issue #3) - Add test_cross_layer_error_message_consistency - Verify registry RuntimeError surfaces categorized text through consuming layer - Ensures fix remains effective across error propagation chain - Lines: test_upstream_session_error_categories.py:547-599 4. **Feature Documentation** (Issue #1 - Blocking) - Add comprehensive "Upstream Session Error Diagnostics" section to observability-otel.md - Document all 13 error categories with descriptions and common causes - Include error message format examples - Provide structured logging metadata schema - Add monitoring/alerting examples (Prometheus, Datadog, Splunk) - Explain ExceptionGroup unwrapping behavior - Lines: observability-otel.md:651-756 All tests pass (18/18). Code review findings fully addressed. Related: #5608 Signed-off-by: Bogdan-Marius-Catanus <bogdan-marius.catanus@ibm.com>
Sagar-Mondal
pushed a commit
to Sagar-Mondal/mcp-context-forge
that referenced
this pull request
Jul 30, 2026
…BM#5631) * feat(observability): improve upstream MCP session error diagnostics Enhance error handling in upstream_session_registry to provide actionable, specific error messages when upstream MCP session creation fails. Replaces generic 'unhandled errors in a TaskGroup' message with categorized errors. Key improvements: - Unwrap ExceptionGroup before string conversion to preserve root cause - Categorize errors into 13 distinct types (connection_refused, timeout, ssl_tls, auth_unauthorized, auth_forbidden, dns_resolution, etc.) - Add structured logging with error_category, exception_type, and metadata for correlation across log aggregation systems - Include full traceback via exc_info for deep diagnosis - Surface error category in RuntimeError message for user visibility This enables operators to quickly identify whether failures are due to: - Network issues (connection refused/reset, DNS, timeouts) - Authentication problems (401/403) - SSL/TLS certificate issues - Upstream server errors (5xx) Error messages now display as: [connection_refused] ConnectionRefusedError: Connection refused [auth_unauthorized] HTTPStatusError: 401 Unauthorized [ssl_tls] SSLError: certificate verify failed [timeout] TimeoutError: Session initialization timeout Benefits: - Faster MTTR for production incidents - Actionable error messages without requiring verbose logging - Consistent error handling across MCP session modes - Better alerting and monitoring capabilities Testing: - All 68 existing upstream_session_registry tests pass - 10 new tests validate error categorization for all failure modes - Backward compatible (still raises RuntimeError) Closes IBM#5608 Signed-off-by: Bogdan-Marius-Catanus <bogdan-marius.catanus@ibm.com> * test(observability): improve structured logger exception handling test coverage Enhance test_structured_logger_exception_handling to ensure lines 462-465 are covered by properly mocking get_structured_logger at the module level where it's imported. The test now verifies that: - The structured logger is actually invoked (confirming except block is hit) - Primary error message is preserved when structured logging fails - Structured logger failures don't disrupt the main error flow Also fix f-string formatting in upstream_session_registry.py per ruff format. This brings coverage of the structured logging exception handler to 100%. Signed-off-by: Bogdan-Marius-Catanus <bogdan-marius.catanus@ibm.com> * update .secrets.baseline Signed-off-by: Bogdan-Marius-Catanus <bogdan-marius.catanus@ibm.com> * update .secrets.baseline Signed-off-by: Bogdan-Marius-Catanus <bogdan-marius.catanus@ibm.com> * feat(observability): address code review findings for upstream session error diagnostics Address all blocking issues and warnings from code review: 1. **Type Safety Enhancement** (Issue IBM#5) - Improve type: ignore comment with detailed rationale - Explain why type checker cannot infer concrete Exception after unwrapping - Lines: upstream_session_registry.py:387-391 2. **Logging Diagnostics Test Coverage** (Issue IBM#2) - Add test_logger_error_call_with_exc_info: verify logger.error called with exc_info - Add test_structured_logger_metadata_payload: verify structured logger metadata - Tests validate both standard logging (with traceback) and structured logging paths - Lines: test_upstream_session_error_categories.py:459-543 3. **Cross-Layer Consistency Regression Test** (Issue IBM#3) - Add test_cross_layer_error_message_consistency - Verify registry RuntimeError surfaces categorized text through consuming layer - Ensures fix remains effective across error propagation chain - Lines: test_upstream_session_error_categories.py:547-599 4. **Feature Documentation** (Issue IBM#1 - Blocking) - Add comprehensive "Upstream Session Error Diagnostics" section to observability-otel.md - Document all 13 error categories with descriptions and common causes - Include error message format examples - Provide structured logging metadata schema - Add monitoring/alerting examples (Prometheus, Datadog, Splunk) - Explain ExceptionGroup unwrapping behavior - Lines: observability-otel.md:651-756 All tests pass (18/18). Code review findings fully addressed. Related: IBM#5608 Signed-off-by: Bogdan-Marius-Catanus <bogdan-marius.catanus@ibm.com> * fix(observability): address code review findings for upstream session error diagnostics This commit implements all feedback from code review, addressing both blocking issues and suggested improvements to the upstream MCP session error diagnostics enhancement. ## Blocking Issues Fixed ### 1. Credential Sanitization (Security) - **Problem**: Exception messages with URLs containing secrets (API keys, tokens) were flowing unsanitized to client-facing RuntimeError messages, logs, and structured logging sinks - **Fix**: Use `sanitize_exception_message()` in `_categorize_upstream_error()` to redact sensitive query params before returning sanitized message - **Coverage**: Added regression tests for API key and Bearer token redaction ### 2. httpx Exception Type Categorization - **Problem**: Real httpx timeout/connection exceptions (httpx.ConnectTimeout, httpx.ReadTimeout, httpx.ConnectError) fell through to 'unknown' category - **Fix**: Check `isinstance(root_cause, httpx.TimeoutException)` to catch all httpx timeout types; handle httpx.ConnectError with message inspection for 'refused' vs generic connection errors - **Coverage**: Added regression tests for httpx.ConnectTimeout, httpx.ReadTimeout, and httpx.ConnectError with/without "refused" message ## Improvements Implemented ### Refactoring - Extracted error categorization logic into pure function `_categorize_upstream_error()` - Returns: (error_category, exception_type, sanitized_message, exception_count) - Makes taxonomy testable as pure function (no async task/transport mocking needed) - Enables future code reuse by tool_service.py for consistency ### Structured Logging Enhancements - Added `correlation_id` from request context for cross-layer correlation - Changed `metadata={...}` to `error_details={...}` for consistency with tool_service.py - `error_details` maps to dedicated column; `metadata` contains non-error context - Added debug logging for structured logger failures (was silent `except: pass`) ### Error Category Additions - Added `mcp_protocol_error` category for McpError (failed session.initialize()) - Tightened `ssl.SSLError` check to use isinstance() before string matching - Fixed httpx.ConnectError to categorize as 'connection_refused' when message contains "refused" ### Log Level Handling - Downgraded post-ready errors (teardown races) to WARNING level - Pre-ready failures remain ERROR (blocks session creation) - Only ERROR-level failures trigger structured logging (reduces alert noise) ### Exception Group Metadata - Track and log `exception_count` when BaseExceptionGroup contains multiple exceptions - Append " (N exceptions in group)" to log messages when count > 1 - Include `exception_count` in structured logging `error_details` ### Documentation Updates - Removed obsolete `MCP_SESSION_POOL_ENABLED` references from observability-otel.md - Updated to describe actual trigger: Mcp-Session-Id header presence - Added `mcp_protocol_error` to error categories table ## Test Coverage ### New Regression Tests (9 added) 1. `test_httpx_connect_timeout_category` - httpx.ConnectTimeout → timeout 2. `test_httpx_read_timeout_category` - httpx.ReadTimeout → timeout 3. `test_httpx_connect_error_with_refused_message` - "refused" → connection_refused 4. `test_httpx_connect_error_generic` - no "refused" → connection_error 5. `test_credential_sanitization_in_http_error` - API key redaction 6. `test_credential_sanitization_with_bearer_token` - Bearer token redaction 7. `test_mcp_protocol_error_category` - McpError → mcp_protocol_error 8. `test_ssl_error_category_with_isinstance_check` - ssl.SSLError via isinstance 9. `test_exception_group_with_multiple_exceptions_logged` - exception_count > 1 ### Modified Tests (2 updated) - `test_structured_logger_metadata_payload` - validates error_details structure - `test_post_ready_error_is_warning_level` - documents WARNING downgrade behavior ### Test Results - All 28 tests in test_upstream_session_error_categories.py pass - All 68 tests in test_upstream_session_registry.py pass (no regressions) - Total: 96 tests passing ## Security Impact **CRITICAL**: This fix prevents credential disclosure that was introduced by the original PR. Before this fix, URLs with secrets in query params (e.g., `?apiKey=secret123`) would leak to client-facing error messages. The sanitization now redacts all sensitive query params using static fallback patterns (api_key, token, password, etc.) and supports gateway-specific param names when available. ## Implementation Notes ### auth_query_params Threading Decision The review suggested threading gateway `auth_query_params_decrypted` through SessionCreateRequest for full credential redaction. However: - Static fallback in `sanitize_exception_message()` already covers common cases (api_key, token, password, Bearer tokens, etc.) - Threading through would require larger changes (SessionCreateRequest fields, all call sites, decryption at registry level) - Current implementation documents this trade-off in code comments ### Story 2 Consistency (tool_service.py) The refactored `_categorize_upstream_error()` function is now available for tool_service.py to call in a future PR, which would deliver full consistency across both session paths. This PR focuses on the registry path where the issue was reported. Closes feedback items from code review on IBM#5608 Signed-off-by: Bogdan-Marius-Catanus <bogdan-marius.catanus@ibm.com> * fix linter issues Signed-off-by: Bogdan-Marius-Catanus <bogdan-marius.catanus@ibm.com> * fix(upstream): handle real-world httpx exception chains and timeout paths Fixes three blocking issues in upstream session error categorization identified during real-socket e2e verification: 1. **connection_refused** detection: httpx.ConnectError wraps ConnectionRefusedError deep in __context__/__cause__ chain. Added _find_in_chain() helper to unwrap exception chains so real refused connections produce correct category instead of generic connection_error. 2. **ssl_tls** detection: Moved ssl.SSLError check to unwrap exception chains (httpx.ConnectError wrapping SSLError) and added fallback chain walk at end of categorization logic. 3. **timeout** at asyncio.wait_for call site: Owner task receives CancelledError (BaseException, excluded from except Exception), so asyncio.wait_for timeout never hit the owner's exception handler. Added explicit except asyncio.TimeoutError block at call site with categorization, sanitization, structured logging, and RuntimeError wrapping to match owner-task error path. 4. **Credential leak via exc_info**: Removed exc_info=exc from all logger.error/warning calls. Python's traceback formatter renders the raw exception __str__, bypassing sanitized message strings and reintroducing credential disclosure for HTTPStatusError. Added end-to-end integration tests (test_upstream_session_error_e2e.py) that validate fixes against real TCP sockets, blackhole listeners, and HTTP servers with no mocking. Updated test expectations: - test_logger_error_call_with_exc_info renamed to test_logger_error_call_without_exc_info and flipped assertion - test_default_session_factory_cancelled_path_runs_on_ready_timeout now expects RuntimeError wrapping TimeoutError with categorization All unit tests (86) and e2e tests (4) passing. Closes IBM#5608 Signed-off-by: Bogdan-Marius-Catanus <bogdan-marius.catanus@ibm.com> * fix(tests): filter caplog records by ERROR level in test_remove_root_generic_exception The test was failing after rebase because it captured all mcpgateway logs, including DEBUG level logs from get_db and user request logging. Updated the assertion to filter caplog records to only include ERROR level logs, ensuring only the expected "Failed to remove root" error message is validated. Signed-off-by: Bogdan-Marius-Catanus <bogdan-marius.catanus@ibm.com> * fix linter issues Signed-off-by: Bogdan-Marius-Catanus <bogdan-marius.catanus@ibm.com> * test: add coverage for uncovered lines in upstream_session_registry.py Add tests to cover previously uncovered lines: - Line 311: _find_in_chain return current path - Line 329: connection_refused fallback message check - Lines 375-377: SSL error detection via _find_in_chain - Line 542: WARNING level log post-ready - Lines 665-666: structured logging exception during timeout Coverage improved from 91.7% to higher coverage for upstream session registry. Tests added: - test_categorize_upstream_error_ssl_error_in_exception_chain - test_categorize_upstream_error_connection_refused_message_fallback - test_categorize_upstream_error_find_in_chain_returns_current - test_default_session_factory_logs_warning_on_post_ready_failure - test_default_session_factory_timeout_structured_logging_failure Signed-off-by: Bogdan-Marius-Catanus <bogdan-marius.catanus@ibm.com> * fixed pre-commit Signed-off-by: Jitesh Nair <jiteshnair@ibm.com> * fixed pre-commit Signed-off-by: Jitesh Nair <jiteshnair@ibm.com> --------- Signed-off-by: Bogdan-Marius-Catanus <bogdan-marius.catanus@ibm.com> Signed-off-by: Jitesh Nair <jiteshnair@ibm.com> Co-authored-by: Bogdan-Marius-Catanus <bogdan-marius.catanus@ibm.com> Co-authored-by: Jitesh Nair <jiteshnair@ibm.com>
prakhar-singh1928
added a commit
that referenced
this pull request
Jul 31, 2026
Duration unit (finding #3): - Rename cpex.control.duration -> cpex.control.duration_ns in aggregate(), _per_control_attributes(), and both sinks to follow OTel unit-suffix convention. Rename rules targeting 'cpex.control.duration_ns' are now semantically correct. Missing ControlExecutionRecord fields (finding #2): - Add cpex.control.plugin_id, cpex.control.plugin_kind, cpex.control.matched, cpex.control.applied, cpex.control.payload_modified to per-control result spans. - Add cpex.control.result.requested_allowed (emitted only when requested_allow != None). results_count semantics (finding #6): - Fix aggregate() results_count to reflect min(accumulated, max_results) — the number of records exported after the per-invocation cap, per #5785 spec. invocation_count semantics (finding #6): - Count only controls that actually ran (completed/error/timeout); exclude skipped/disabled/cancelled per #5785 aggregation spec. Timeout post-hook gap (finding #5): - Add ctl_acc parameter to _run_timeout_post_invoke(); all four call sites now pass ctl_acc=_ctl_acc so timeout post-hook execution records feed the per-invocation accumulator and appear in the emitted telemetry. Fail-closed ValueError fallback (finding #8b): - apply_attribute_mapping() now returns attributes unchanged when compile_attribute_policy raises ValueError (e.g. otel.* destination, empty key, key > 256 chars). Previously it constructed partially-validated exact mappings that could bypass the otel.* guard — now fail-closed. CPEX denial path documentation (finding #4): - Add explanatory comment in except (PluginError, PluginViolationError) block documenting the upstream CPEX 0.1.2 framework gap: when violations_as_exceptions=True, CPEX raises before appending the denying plugin's ControlExecutionRecord, so _ctl_acc captures pre-denial records only. The effective_allow=False outcome is preserved via pre_denied flag. Tests updated for all changes above; 741 passed, 33 skipped. Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
prakhar-singh1928
added a commit
that referenced
this pull request
Jul 31, 2026
Duration unit (finding #3): - Rename cpex.control.duration -> cpex.control.duration_ns in aggregate(), _per_control_attributes(), and both sinks to follow OTel unit-suffix convention. Rename rules targeting 'cpex.control.duration_ns' are now semantically correct. Missing ControlExecutionRecord fields (finding #2): - Add cpex.control.plugin_id, cpex.control.plugin_kind, cpex.control.matched, cpex.control.applied, cpex.control.payload_modified to per-control result spans. - Add cpex.control.result.requested_allowed (emitted only when requested_allow != None). results_count semantics (finding #6): - Fix aggregate() results_count to reflect min(accumulated, max_results) — the number of records exported after the per-invocation cap, per #5785 spec. invocation_count semantics (finding #6): - Count only controls that actually ran (completed/error/timeout); exclude skipped/disabled/cancelled per #5785 aggregation spec. Timeout post-hook gap (finding #5): - Add ctl_acc parameter to _run_timeout_post_invoke(); all four call sites now pass ctl_acc=_ctl_acc so timeout post-hook execution records feed the per-invocation accumulator and appear in the emitted telemetry. Fail-closed ValueError fallback (finding #8b): - apply_attribute_mapping() now returns attributes unchanged when compile_attribute_policy raises ValueError (e.g. otel.* destination, empty key, key > 256 chars). Previously it constructed partially-validated exact mappings that could bypass the otel.* guard — now fail-closed. CPEX denial path documentation (finding #4): - Add explanatory comment in except (PluginError, PluginViolationError) block documenting the upstream CPEX 0.1.2 framework gap: when violations_as_exceptions=True, CPEX raises before appending the denying plugin's ControlExecutionRecord, so _ctl_acc captures pre-denial records only. The effective_allow=False outcome is preserved via pre_denied flag. Tests updated for all changes above; 741 passed, 33 skipped. Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
15 tasks
prakhar-singh1928
added a commit
that referenced
this pull request
Jul 31, 2026
Duration unit (finding #3): - Rename cpex.control.duration -> cpex.control.duration_ns in aggregate(), _per_control_attributes(), and both sinks to follow OTel unit-suffix convention. Rename rules targeting 'cpex.control.duration_ns' are now semantically correct. Missing ControlExecutionRecord fields (finding #2): - Add cpex.control.plugin_id, cpex.control.plugin_kind, cpex.control.matched, cpex.control.applied, cpex.control.payload_modified to per-control result spans. - Add cpex.control.result.requested_allowed (emitted only when requested_allow != None). results_count semantics (finding #6): - Fix aggregate() results_count to reflect min(accumulated, max_results) — the number of records exported after the per-invocation cap, per #5785 spec. invocation_count semantics (finding #6): - Count only controls that actually ran (completed/error/timeout); exclude skipped/disabled/cancelled per #5785 aggregation spec. Timeout post-hook gap (finding #5): - Add ctl_acc parameter to _run_timeout_post_invoke(); all four call sites now pass ctl_acc=_ctl_acc so timeout post-hook execution records feed the per-invocation accumulator and appear in the emitted telemetry. Fail-closed ValueError fallback (finding #8b): - apply_attribute_mapping() now returns attributes unchanged when compile_attribute_policy raises ValueError (e.g. otel.* destination, empty key, key > 256 chars). Previously it constructed partially-validated exact mappings that could bypass the otel.* guard — now fail-closed. CPEX denial path documentation (finding #4): - Add explanatory comment in except (PluginError, PluginViolationError) block documenting the upstream CPEX 0.1.2 framework gap: when violations_as_exceptions=True, CPEX raises before appending the denying plugin's ControlExecutionRecord, so _ctl_acc captures pre-denial records only. The effective_allow=False outcome is preserved via pre_denied flag. Tests updated for all changes above; 741 passed, 33 skipped. Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
prakhar-singh1928
added a commit
that referenced
this pull request
Aug 4, 2026
Duration unit (finding #3): - Rename cpex.control.duration -> cpex.control.duration_ns in aggregate(), _per_control_attributes(), and both sinks to follow OTel unit-suffix convention. Rename rules targeting 'cpex.control.duration_ns' are now semantically correct. Missing ControlExecutionRecord fields (finding #2): - Add cpex.control.plugin_id, cpex.control.plugin_kind, cpex.control.matched, cpex.control.applied, cpex.control.payload_modified to per-control result spans. - Add cpex.control.result.requested_allowed (emitted only when requested_allow != None). results_count semantics (finding #6): - Fix aggregate() results_count to reflect min(accumulated, max_results) — the number of records exported after the per-invocation cap, per #5785 spec. invocation_count semantics (finding #6): - Count only controls that actually ran (completed/error/timeout); exclude skipped/disabled/cancelled per #5785 aggregation spec. Timeout post-hook gap (finding #5): - Add ctl_acc parameter to _run_timeout_post_invoke(); all four call sites now pass ctl_acc=_ctl_acc so timeout post-hook execution records feed the per-invocation accumulator and appear in the emitted telemetry. Fail-closed ValueError fallback (finding #8b): - apply_attribute_mapping() now returns attributes unchanged when compile_attribute_policy raises ValueError (e.g. otel.* destination, empty key, key > 256 chars). Previously it constructed partially-validated exact mappings that could bypass the otel.* guard — now fail-closed. CPEX denial path documentation (finding #4): - Add explanatory comment in except (PluginError, PluginViolationError) block documenting the upstream CPEX 0.1.2 framework gap: when violations_as_exceptions=True, CPEX raises before appending the denying plugin's ControlExecutionRecord, so _ctl_acc captures pre-denial records only. The effective_allow=False outcome is preserved via pre_denied flag. Tests updated for all changes above; 741 passed, 33 skipped. Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
prakhar-singh1928
added a commit
that referenced
this pull request
Aug 4, 2026
Duration unit (finding #3): - Rename cpex.control.duration -> cpex.control.duration_ns in aggregate(), _per_control_attributes(), and both sinks to follow OTel unit-suffix convention. Rename rules targeting 'cpex.control.duration_ns' are now semantically correct. Missing ControlExecutionRecord fields (finding #2): - Add cpex.control.plugin_id, cpex.control.plugin_kind, cpex.control.matched, cpex.control.applied, cpex.control.payload_modified to per-control result spans. - Add cpex.control.result.requested_allowed (emitted only when requested_allow != None). results_count semantics (finding #6): - Fix aggregate() results_count to reflect min(accumulated, max_results) — the number of records exported after the per-invocation cap, per #5785 spec. invocation_count semantics (finding #6): - Count only controls that actually ran (completed/error/timeout); exclude skipped/disabled/cancelled per #5785 aggregation spec. Timeout post-hook gap (finding #5): - Add ctl_acc parameter to _run_timeout_post_invoke(); all four call sites now pass ctl_acc=_ctl_acc so timeout post-hook execution records feed the per-invocation accumulator and appear in the emitted telemetry. Fail-closed ValueError fallback (finding #8b): - apply_attribute_mapping() now returns attributes unchanged when compile_attribute_policy raises ValueError (e.g. otel.* destination, empty key, key > 256 chars). Previously it constructed partially-validated exact mappings that could bypass the otel.* guard — now fail-closed. CPEX denial path documentation (finding #4): - Add explanatory comment in except (PluginError, PluginViolationError) block documenting the upstream CPEX 0.1.2 framework gap: when violations_as_exceptions=True, CPEX raises before appending the denying plugin's ControlExecutionRecord, so _ctl_acc captures pre-denial records only. The effective_allow=False outcome is preserved via pre_denied flag. Tests updated for all changes above; 741 passed, 33 skipped. Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
prakhar-singh1928
added a commit
that referenced
this pull request
Aug 4, 2026
Duration unit (finding #3): - Rename cpex.control.duration -> cpex.control.duration_ns in aggregate(), _per_control_attributes(), and both sinks to follow OTel unit-suffix convention. Rename rules targeting 'cpex.control.duration_ns' are now semantically correct. Missing ControlExecutionRecord fields (finding #2): - Add cpex.control.plugin_id, cpex.control.plugin_kind, cpex.control.matched, cpex.control.applied, cpex.control.payload_modified to per-control result spans. - Add cpex.control.result.requested_allowed (emitted only when requested_allow != None). results_count semantics (finding #6): - Fix aggregate() results_count to reflect min(accumulated, max_results) — the number of records exported after the per-invocation cap, per #5785 spec. invocation_count semantics (finding #6): - Count only controls that actually ran (completed/error/timeout); exclude skipped/disabled/cancelled per #5785 aggregation spec. Timeout post-hook gap (finding #5): - Add ctl_acc parameter to _run_timeout_post_invoke(); all four call sites now pass ctl_acc=_ctl_acc so timeout post-hook execution records feed the per-invocation accumulator and appear in the emitted telemetry. Fail-closed ValueError fallback (finding #8b): - apply_attribute_mapping() now returns attributes unchanged when compile_attribute_policy raises ValueError (e.g. otel.* destination, empty key, key > 256 chars). Previously it constructed partially-validated exact mappings that could bypass the otel.* guard — now fail-closed. CPEX denial path documentation (finding #4): - Add explanatory comment in except (PluginError, PluginViolationError) block documenting the upstream CPEX 0.1.2 framework gap: when violations_as_exceptions=True, CPEX raises before appending the denying plugin's ControlExecutionRecord, so _ctl_acc captures pre-denial records only. The effective_allow=False outcome is preserved via pre_denied flag. Tests updated for all changes above; 741 passed, 33 skipped. Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
prakhar-singh1928
added a commit
that referenced
this pull request
Aug 4, 2026
Duration unit (finding #3): - Rename cpex.control.duration -> cpex.control.duration_ns in aggregate(), _per_control_attributes(), and both sinks to follow OTel unit-suffix convention. Rename rules targeting 'cpex.control.duration_ns' are now semantically correct. Missing ControlExecutionRecord fields (finding #2): - Add cpex.control.plugin_id, cpex.control.plugin_kind, cpex.control.matched, cpex.control.applied, cpex.control.payload_modified to per-control result spans. - Add cpex.control.result.requested_allowed (emitted only when requested_allow != None). results_count semantics (finding #6): - Fix aggregate() results_count to reflect min(accumulated, max_results) — the number of records exported after the per-invocation cap, per #5785 spec. invocation_count semantics (finding #6): - Count only controls that actually ran (completed/error/timeout); exclude skipped/disabled/cancelled per #5785 aggregation spec. Timeout post-hook gap (finding #5): - Add ctl_acc parameter to _run_timeout_post_invoke(); all four call sites now pass ctl_acc=_ctl_acc so timeout post-hook execution records feed the per-invocation accumulator and appear in the emitted telemetry. Fail-closed ValueError fallback (finding #8b): - apply_attribute_mapping() now returns attributes unchanged when compile_attribute_policy raises ValueError (e.g. otel.* destination, empty key, key > 256 chars). Previously it constructed partially-validated exact mappings that could bypass the otel.* guard — now fail-closed. CPEX denial path documentation (finding #4): - Add explanatory comment in except (PluginError, PluginViolationError) block documenting the upstream CPEX 0.1.2 framework gap: when violations_as_exceptions=True, CPEX raises before appending the denying plugin's ControlExecutionRecord, so _ctl_acc captures pre-denial records only. The effective_allow=False outcome is preserved via pre_denied flag. Tests updated for all changes above; 741 passed, 33 skipped. Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
christophercolumbusdog
pushed a commit
to christophercolumbusdog/mcp-context-forge
that referenced
this pull request
Aug 4, 2026
* feat: consume Cpex control execution records Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * fix(plugins): suppress pylint unused-import on cpex feature-detection probe Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * style: align inline comments in control_telemetry.py Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * test(plugins): add targeted coverage tests for control-telemetry and cpex-compat - Hoist all imports to top-level (no inline imports inside test methods) - Add TestAggregateExceptionPath: covers aggregate() inner except path (lines 192-193) - Add TestGetMaxResultsExceptionPath: covers _get_max_results() except branch (lines 484-485) - Add TestBuildFlattenedAttributesEdgeCases: error_code branch, per-record except, outer except (lines 553-555, 561-563) - Add TestEmitDbSpansExceptionPaths: empty-attrs skip (line 327), rollback on start_span failure (lines 342-348), close() failure swallowed (lines 353-354) - Add TestEmitOtelSpansActivePath: verifies OTel summary+result spans emitted when otel_tracing_enabled+otel_context_active (lines 379-387) - Add TestRecordControlTelemetryTruncated: truncated attribute present (line 260) - Add TestRecordControlTelemetryFlatten: flatten_results branch (lines 267-268) - Add TestExecutionRecordsSupportedImportError: ImportError branch (lines 51-52) - Add wildcard rule tests in TestWildcardAttributeMapping: key>256 rejection (line 535), wildcard/exact removal compilation (lines 543-551), segment extraction mismatch (line 593), ValueError fallback (lines 657-660) - Add TestConcurrencyIsolation and TestObservabilityServiceAPICompatibility Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * fix(plugins): address review findings from issue IBM#5785 PR IBM#6003 Duration unit (finding IBM#3): - Rename cpex.control.duration -> cpex.control.duration_ns in aggregate(), _per_control_attributes(), and both sinks to follow OTel unit-suffix convention. Rename rules targeting 'cpex.control.duration_ns' are now semantically correct. Missing ControlExecutionRecord fields (finding IBM#2): - Add cpex.control.plugin_id, cpex.control.plugin_kind, cpex.control.matched, cpex.control.applied, cpex.control.payload_modified to per-control result spans. - Add cpex.control.result.requested_allowed (emitted only when requested_allow != None). results_count semantics (finding IBM#6): - Fix aggregate() results_count to reflect min(accumulated, max_results) — the number of records exported after the per-invocation cap, per IBM#5785 spec. invocation_count semantics (finding IBM#6): - Count only controls that actually ran (completed/error/timeout); exclude skipped/disabled/cancelled per IBM#5785 aggregation spec. Timeout post-hook gap (finding IBM#5): - Add ctl_acc parameter to _run_timeout_post_invoke(); all four call sites now pass ctl_acc=_ctl_acc so timeout post-hook execution records feed the per-invocation accumulator and appear in the emitted telemetry. Fail-closed ValueError fallback (finding #8b): - apply_attribute_mapping() now returns attributes unchanged when compile_attribute_policy raises ValueError (e.g. otel.* destination, empty key, key > 256 chars). Previously it constructed partially-validated exact mappings that could bypass the otel.* guard — now fail-closed. CPEX denial path documentation (finding IBM#4): - Add explanatory comment in except (PluginError, PluginViolationError) block documenting the upstream CPEX 0.1.2 framework gap: when violations_as_exceptions=True, CPEX raises before appending the denying plugin's ControlExecutionRecord, so _ctl_acc captures pre-denial records only. The effective_allow=False outcome is preserved via pre_denied flag. Tests updated for all changes above; 741 passed, 33 skipped. Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * fix gaps Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * fix(plugins): account for tier-3 export-cap drops in cpex.control.truncated Records beyond CPEX_CONTROL_TELEMETRY_MAX_RESULTS were silently discarded by islice() in _emit_db_spans() and _emit_otel_spans() without being counted in the cpex.control.truncated summary attribute, leaving downstream operators with an incomplete truncation indicator. Fix: - Add _export_cap_dropped field to ControlTelemetryAccumulator - Add mark_export_cap_dropped(count) method called once per emit in record_control_telemetry() before building the summary span - truncated property now returns _truncated + _export_cap_dropped covering all three tiers: Tier 1: per-hook cap (MAX_RECORDS_PER_HOOK = 64) Tier 2: per-call accumulation cap (MAX_RECORDS_PER_CALL = 128) Tier 3: export cap (CPEX_CONTROL_TELEMETRY_MAX_RESULTS, default 32) Tests: - Add TestExportCapTruncation (5 tests) in test_control_telemetry.py - Update TestRecordControlTelemetryTruncated to assert correct total across all three tiers (2 tier-1/2 drops + 96 tier-3 drops = 98) Relates to IBM#5785 Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * fix linting Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * fix(plugins): address blocking review findings on cpex control telemetry PII / agent.id (blocking): - Gate cpex.control.agent.id behind CPEX_CONTROL_TELEMETRY_EMIT_AGENT_ID (default false). Adds _emit_agent_id_enabled() helper and new config field with explicit PII/GDPR warning in description and .env.example. config_keys amplification (functionally impacting): - Add _MAX_CONFIG_KEY_LEN=128 per-key cap and _MAX_CONFIG_KEYS_JOINED_LEN=4096 total joined-string cap. Applied in both _per_control_attributes() and _build_flattened_attributes() to prevent large/adversarial key names causing telemetry amplification or data leakage. Default-off (functionally impacting): - Change CPEX_CONTROL_TELEMETRY_ENABLED default from true to false. Each traced tool call creates up to 1 summary + MAX_RESULTS result DB spans; operators must explicitly opt in after reviewing storage/cardinality impact. E2E tests set CPEX_CONTROL_TELEMETRY_ENABLED=true explicitly via os.environ. E2E denial-path tests (test coverage): - Add denying_plugin_app fixture: PIIFilter with block_on_detection=true and enforcement_mode=pre_invoke. - Add TestDenialPathTelemetry with two tests: test_pre_invoke_denial_summary_span_allowed_false: verifies result.allowed=false and enforcement_point=pre when a real plugin denies on pre_invoke. test_pre_invoke_denial_tool_never_executed: verifies tool arguments do not leak into control telemetry attributes on the denial path. Both tests skip gracefully when PIIFilter does not detect PII (environment variation). Unit test fixes: - Add _enabled_settings() helper in test_record_control_telemetry.py so all TestRecordControlTelemetryDB tests work correctly with the new default=false. - Patch cfg_mod.settings in all DB-sink tests and TestRecordControlTelemetryTruncated. Relates to IBM#5785, PR IBM#6003 Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * addressed comments Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * fixed secrets Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * fix(plugins): address four follow-up review findings on cpex control telemetry Fix 1 — wildcard insertion order corrupted by cache key sorting: - apply_attribute_mapping() was building its cache key with sorted(mapping.items()), which reordered wildcard rules and changed which match fired first. The documented contract is 'first matching wildcard wins' (compile_attribute_policy evaluates rules in insertion order). Changed to tuple(mapping.items()) to preserve the caller's declared rule order. - Regression test: TestWildcardInsertionOrderPreserved (3 cases) in test_record_control_telemetry.py. Fix 2 — config_keys joined cap was char-based, not byte-aware: - Both projections (_per_control_attributes and _build_flattened_attributes) used joined[:_MAX_CONFIG_KEYS_JOINED_LEN] which counts Unicode code points, not UTF-8 bytes. Multibyte key names (e.g. CJK characters) could therefore exceed the intended byte budget. - Fixed both sites to use _safe_str(joined, _MAX_CONFIG_KEYS_JOINED_LEN) which is already byte-aware (encodes to UTF-8 before slicing). - Regression tests: TestConfigKeysByteCapAware (2 cases) covering multibyte input and ASCII round-trip. Fix 3 — _enforcement_point returned 'none' on denial with empty records: - When CPEX raises PluginViolationError with violations_as_exceptions=True it does so before appending a ControlExecutionRecord, so the accumulator can have no records even though a pre-hook fired. _enforcement_point was only inspecting acc.records and returned 'none' in this case. - Fixed to also check acc.pre_denied / acc.post_denied flags so a first-plugin pre-invoke denial correctly reports enforcement_point='pre'. - E2E denial test tightened: removed 'none' from accepted enforcement_point values; removed pytest.skip on missing summary span (absence is a regression); removed 500 from accepted status codes (indicates unhandled exception). - Unit tests: TestEnforcementPointDenialFlags (3 cases). Fix 4 — PluginError on first plugin emits no telemetry: - When the first plugin in the chain raises PluginError the accumulator has no records and no denial flags, so record_control_telemetry() returned early at the empty-accumulator guard, emitting nothing. Plugin outages were invisible in observability dashboards. - Added _plugin_errored field and mark_plugin_error() method to ControlTelemetryAccumulator. The outer except PluginError handler in tool_service.py now calls mark_plugin_error() before record_control_telemetry(). - Empty-accumulator guard now includes plugin_errored so first-plugin failures always emit a summary span. - aggregate() emits cpex.control.plugin_error=True when the flag is set, making plugin outages distinguishable from both successful allows and policy denials. - effective_allowed remains True on PluginError (intentional — outage != denial). - Unit tests: TestMarkPluginError (6 cases) in test_control_telemetry.py. 155 unit tests pass; ruff and pylint 10.00/10. Relates to IBM#5785, PR IBM#6003 Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * fix(plugins): address blocking and warning findings from ja8zyjits review Blocking 1: CHANGELOG.md CPEX_CONTROL_TELEMETRY_ENABLED default 'true' → 'false' to match mcpgateway/config.py:1987 (default=False). Operators would have expected the feature on by default when it is off. Blocking 2: mark_export_cap_dropped() idempotency — changed from += to a first-write-wins assignment guarded by _export_cap_dropped == 0. Subsequent calls on the same accumulator are now no-ops, preventing cpex.control.truncated inflation on retry paths or test reuse. Two regression tests added (TestExportCapTruncation::test_mark_export_cap_dropped_is_idempotent and ::test_mark_export_cap_dropped_zero_then_positive). Warning 1: Replaced undefined 'S4 principles' reference in control_telemetry.py module docstring with an explicit description: 'bounded cardinality and field-level sanitization ... _safe_str/_safe_num helpers and field-name allowlists' pointing to plugins/utils.py. Warning 2: Extracted the 5 repeated record_control_telemetry() call blocks in tool_service.py::invoke_tool() into a single nested helper _emit_ctl_telemetry() that closes over _ctl_acc, name, app_user_email, user_email, gateway_name, and server_id. Identity mapping rationale previously inline at the normal-return site is now in the helper docstring; exception-path sites are reduced to a single _emit_ctl_telemetry() call with a back-reference comment. 145 plugin unit tests pass; ruff all-clear; pylint 10.00/10. Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * fix(plugins): sanitize config_keys and make plugin_error result.allowed indeterminate config_keys sanitization (vishu-bh finding): - Add _sanitize_config_key() helper that validates each key against _CONFIG_KEY_RE (^[A-Za-z0-9_.-]{1,64}$). Rejects commas (the CSV delimiter), CR/LF/NUL/TAB (log-injection vectors), non-ASCII, and secret-shaped tokens containing '='. Keys that fail are dropped entirely rather than truncated, preventing ambiguous joined output. - Both _per_control_attributes() and _build_flattened_attributes() now use the validator. When every key in a record fails, the attribute is omitted rather than emitting an empty string. - 14 new tests in TestSanitizeConfigKey: comma, CR, LF, NUL, TAB, non-ASCII, secret-marker, empty, oversized, max-length, and three end-to-end tests through _per_control_attributes(). PluginError indeterminate decision (vishu-bh finding): - mark_plugin_error() now accepts hook='pre'|'post'|'' and stores it in _plugin_error_hook for enforcement_point derivation. - aggregate() omits cpex.control.result.allowed entirely when plugin_errored=True and no denial flag is set (decision is indeterminate — the chain did not complete). When both plugin_errored and a denial flag are set, denial takes precedence and result.allowed is emitted as False. - _enforcement_point() gains a third fallback tier: plugin_error_hook, so a first-plugin crash still reports the correct enforcement point rather than 'none'. - tool_service.py: _ctl_last_hook tracks 'pre'/'post' across all invoke sites; outer except PluginError passes hook=_ctl_last_hook. - 9 new tests in TestPluginErrorIndeterminate. - Updated TestMarkPluginError::test_plugin_error_flag_bypasses... to assert result.allowed is ABSENT (was: True) for the first-plugin error path. 849 plugin unit tests pass; ruff all-clear; pylint 10.00/10. Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * fixed ruff format Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * fix: tolerate stale merge-queue ref on SARIF upload When a merge-queue attempt is ejected (e.g. transient Docker cache error) the ephemeral ref refs/heads/gh-readonly-queue/main/pr-* is deleted. Any retried attempt carries the same stale ref baked into the job context, so codeql-action/upload-sarif fails with 'ref not found', failing fedramp-compliance and scan jobs and ejecting the queue again -- workflow_run_attempt 1, 2, and 3 all hit this loop. Add continue-on-error: true to both Upload SARIF to CodeQL steps so a stale-ref rejection from the CodeQL API does not fail the job. The scan result is unaffected; SARIF lands on the push-to-main run that follows a successful merge. Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * update secrets Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> * undo .yml change Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com> --------- Signed-off-by: prakhar-singh1928 <prakhar.singh1928@ibm.com>
8 tasks
14 tasks
Open
15 tasks
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
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.