Skip to content

fix(extensions): keep TLS causes in MCP connection diagnostics - #294

Merged
Lucas1479 merged 3 commits into
Code-Amadeus:mainfrom
NopeCharon:fix/mcp-probe-tls-cause
Nov 5, 2026
Merged

Lucas1479 merged 3 commits into
Code-Amadeus:mainfrom
NopeCharon:fix/mcp-probe-tls-cause

Conversation

@NopeCharon

@NopeCharon NopeCharon commented Nov 5, 2026 •

Copy link
Copy Markdown
Contributor

What and why

The MCP connection probe added in #291 classifies every httpx2.ConnectError as connection_unreachable. httpcore2 maps an ssl.SSLError raised during the TLS handshake (including certificate verification failures) to ConnectError. The mapped exception survives in the exception chain, including __context__ after the connection pool re-raises with from None, so an HTTPS server with a self-signed, expired or mismatched certificate was reported as "Could not reach the MCP server" and the certificate reason was lost in both the plugin card and Settings → Providers → MCP (details in #292).

_probe_error now unwraps only the httpx2/httpcore2 ConnectError layers and checks whether the mapped failure is an ssl.SSLError. Genuine certificate/protocol errors fall through to the existing connection_failed result with the redacted underlying diagnostic. Other connection failures, including a refused connection or a peer closing during the TLS handshake, keep the actionable unreachable message. In particular, the classifier does not look through anyio's EndOfStream/BrokenResourceError to an incidental SSL error retained in their context. TLS validation is unchanged.

Fixes #292

Linked Issue for product-semantic or public-contract changes: none (routine fix)

Change class

  • Routine fix, documentation, test, maintenance, or presentation-only UI
  • Product-semantic or public-contract change discussed in the linked Issue
  • Isolated, default-off experiment

Owning layer: server/handlers/mcp_connection_handler.py, the Host handler that classifies mcp.connection.test probe failures for both the plugin card and Settings → Providers → MCP.

User-visible effect, or none: a TLS failure while testing an HTTPS MCP connection shows its certificate reason (for example CERTIFICATE_VERIFY_FAILED ...) again, instead of the "check that it is running and that the URL and port are correct" message.

Compatibility or migration impact, or none: none. TLS failures return the connection_failed code they returned before #291; other codes are unchanged.

Evidence

Commands and manual journeys run:

  • python -m pytest -q tests/test_mcp_connections.py: 17 passed (macOS, CPython 3.12, mcp==2.2.0, httpx2/httpcore2 2.12.0).

  • The new test test_http_probe_keeps_tls_certificate_diagnostics_without_credentials runs the handler through the real MCP client with a transport that raises a ConnectError caused by ssl.SSLCertVerificationError. It fails on main (connection_unreachable) and passes with this change; it also checks the bearer token is redacted from the detail.

  • The existing unreachable case now raises its ConnectError from a ConnectionRefusedError, so it checks that a non-TLS OSError cause still gives connection_unreachable.

  • Manual check without mocks: tested the handler against a local HTTPS server with a self-signed certificate and against a closed port. On main both returned connection_unreachable. With this change the HTTPS server returns connection_failed with the certificate-not-trusted reason, and the closed port still returns connection_unreachable.

  • ruff check server/handlers/mcp_connection_handler.py tests/test_mcp_connections.py and git diff --check: passed.

  • Relevant Python tests pass

  • CPU/model-less baseline remains supported

  • Electron npm run build passes when Electron code changed

  • Dependency audit passes when dependencies changed

  • Before/after screenshots are attached for visible UI changes

  • Documentation/examples are updated for changed settings or contracts

Maintainer follow-up validation (0a8d7046, Windows, CPython 3.12.10, MCP 2.2.0, httpx2/httpcore2 2.12.0, anyio 4.14.2):

  • Added certificate-free loopback cases using the real HTTP/TLS stack: an HTTP response to ClientHello preserves the SSL diagnostic; a peer that reads ClientHello and closes retains connection_unreachable. The tests bypass ambient proxies, bound fixture lifetime, and await accepted connections during cleanup.
  • Before/after: the protocol-error case fails on the main baseline, and the peer-close case fails on the original PR head. Both pass after narrowing the classifier.
  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 python -m pytest -p pytest_asyncio.plugin -q tests/test_mcp_connections.py: 19 passed. Existing certificate-diagnostic and credential-redaction coverage remains in place.
  • python -m ruff check server/handlers/mcp_connection_handler.py tests/test_mcp_connections.py and git diff --check: passed.

Final check

  • This PR addresses one coherent problem without unrelated cleanup
  • It does not add a speculative API, fallback, or compatibility path
  • No secrets, local state, model weights, voice material, or restricted assets are included
  • Third-party notices and provenance are preserved

The MCP connection probe classified every httpx2 ConnectError as
connection_unreachable. httpcore2 maps ssl.SSLError raised during the
TLS handshake, including certificate verification failures, to
ConnectError, so an HTTPS endpoint with a self-signed, expired or
mismatched certificate was reported as "Could not reach the MCP
server" and the certificate reason was lost in both the plugin card
and Settings -> Providers -> MCP.

A ConnectError whose cause chain contains an ssl.SSLError now falls
through to the existing connection_failed result, whose detail is the
redacted underlying error. Ordinary connection failures, including
those caused by other OSErrors such as a refused connection, keep the
actionable unreachable message. TLS validation itself is unchanged.

Fixes Code-Amadeus#292
@NopeCharon
NopeCharon requested a review from Lucas1479 as a code owner November 5, 2026 16:29
@Lucas1479

Copy link
Copy Markdown
Member

Thanks for the fix. I added the maintainer follow-up in 0a8d704 after verifying the real transport exception chain.

The original walker also reached an SSLEOFError retained underneath anyio's EndOfStream when a peer closed after receiving ClientHello. That changed the useful unreachable hint into the bare detail ConnectError. The classifier now unwraps only httpx2/httpcore2 ConnectError layers and examines the mapped failure. It still follows __context__, which is necessary because httpcore2's connection pool re-raises with from None.

Added two certificate-free loopback cases using the real HTTP/TLS stack, with proxy isolation and bounded, awaited fixture cleanup:

  • An HTTP response to ClientHello retains the TLS protocol diagnostic. This case fails on the main baseline.
  • A peer closing after ClientHello retains connection_unreachable. This case fails on the original PR head.

Both now pass, alongside the original certificate/redaction tests: 19 passed on Windows / CPython 3.12.10. Ruff and git diff --check also pass. The PR description now documents the actual exception chain.

The new CI run exposed a separate infrastructure blocker: GitHub has retired macos-14, so the macOS jobs are cancelled before checkout. That migration is isolated in #295; I will finish review and merge after the updated base and CI are verified.

Lucas1479 added a commit that referenced this pull request Nov 5, 2026
## What and why

GitHub retired the macOS 14 hosted images on November 2, 2026. The macOS
voice and MPS jobs now stop before checkout with `The configuration
'macos-14-arm64-us-default' is not supported`, including a targeted
retry while validating #294.

Move those two jobs to the supported macOS 15 Apple Silicon image and
update the matching installation-profile reference. This restores the
existing CI coverage without changing application requirements or
dependency pins. See the [official retirement
notice](actions/runner-images#13518).

Linked Issue for product-semantic or public-contract changes: none (CI
maintenance).

## Change class

- [x] Routine fix, documentation, test, maintenance, or
presentation-only UI
- [ ] Product-semantic or public-contract change discussed in the linked
Issue
- [ ] Isolated, default-off experiment

Owning layer: GitHub Actions runner selection.
User-visible effect: none in the application; macOS validation can
execute again.
Compatibility or migration impact: CI uses macOS 15 instead of the
retired macOS 14 image.

## Evidence

- Parsed both workflow files as YAML and verified the voice/MPS runner
selections.
- `git diff --check`: passed.
- All 15 GitHub Actions checks passed, including the macOS 15 voice
workflow and MPS installation candidate, Linux contracts, Windows Python
suites, Electron builds, and packaged application smoke/recovery checks.

@Lucas1479 Lucas1479 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed final head 4151636d039246c85ed67c83ade4526f6a2d4564. The narrowed classifier preserves certificate/protocol diagnostics while keeping the actionable unreachable hint for a peer closing during the handshake. The real loopback tests cover both sides, and the original credential-redaction coverage remains in place.

Validation: the MCP suite passes 19/19 in Windows CI, and all 15/15 checks are green on the latest main after the separate runner migration in #295. Windows shard 2 required one targeted rerun after stalling in the unchanged Codex adapter suite; that suite passed 74/74 on the rerun (and locally). No remaining review findings.

@Lucas1479
Lucas1479 merged commit 53f8f4f into Code-Amadeus:main Nov 5, 2026
19 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Preserve TLS certificate causes in MCP connection diagnostics

2 participants