Improve MCP TLS error reporting - #164
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| except SearchClientError as e: | ||
| return _error_response(e, "flights", message=f"Search failed: {e}") |
There was a problem hiding this comment.
except SearchClientError block prefixes every typed error message with "Search failed: ", including SearchCertificateError whose message is already a complete, actionable sentence. The prefix is redundant and makes the error field slightly awkward when presented verbatim to MCP clients. Passing the exception message directly (like ParseError does) keeps it clean.
| except SearchClientError as e: | |
| return _error_response(e, "flights", message=f"Search failed: {e}") | |
| except SearchClientError as e: | |
| return _error_response(e, "flights") |
Prompt To Fix With AI
This is a comment left during a code review.
Path: fli/mcp/server.py
Line: 532-533
Comment:
The `except SearchClientError` block prefixes every typed error message with `"Search failed: "`, including `SearchCertificateError` whose message is already a complete, actionable sentence. The prefix is redundant and makes the `error` field slightly awkward when presented verbatim to MCP clients. Passing the exception message directly (like `ParseError` does) keeps it clean.
```suggestion
except SearchClientError as e:
return _error_response(e, "flights")
```
How can I resolve this? If you propose a fix, please make it concise.
| except SearchClientError as e: | ||
| return _error_response(e, "dates", message=f"Search failed: {e}") |
There was a problem hiding this comment.
SearchCertificateError (and other typed client errors) already carry self-contained messages, so the "Search failed: " wrapper adds noise without value.
| except SearchClientError as e: | |
| return _error_response(e, "dates", message=f"Search failed: {e}") | |
| except SearchClientError as e: | |
| return _error_response(e, "dates") |
Prompt To Fix With AI
This is a comment left during a code review.
Path: fli/mcp/server.py
Line: 633-634
Comment:
Same redundant prefix for the date-search path: `SearchCertificateError` (and other typed client errors) already carry self-contained messages, so the `"Search failed: "` wrapper adds noise without value.
```suggestion
except SearchClientError as e:
return _error_response(e, "dates")
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| def _ca_bundle_from_env() -> str | None: | ||
| """Return the first configured CA bundle path from supported environment variables.""" | ||
| for name in _CA_BUNDLE_ENV_VARS: | ||
| value = os.environ.get(name) | ||
| if value: | ||
| return value | ||
| return None |
There was a problem hiding this comment.
_ca_bundle_from_env() returns any non-empty string verbatim. If the user sets FLI_CA_BUNDLE to a path that does not exist or is not readable by curl, the failure happens at request time as a low-level curl error. That error won't match CertificateVerifyError in _wrap_request_error, so it falls through to the generic SearchClientError fallback with error_type: "search_error" — exactly the opaque behavior this PR set out to improve. A quick os.path.isfile check or a startup-time ValueError similar to the existing FLI_TIMEOUT validation would surface the misconfiguration clearly.
Prompt To Fix With AI
This is a comment left during a code review.
Path: fli/search/client.py
Line: 78-84
Comment:
**No validation that the CA bundle path is readable**
`_ca_bundle_from_env()` returns any non-empty string verbatim. If the user sets `FLI_CA_BUNDLE` to a path that does not exist or is not readable by curl, the failure happens at request time as a low-level curl error. That error won't match `CertificateVerifyError` in `_wrap_request_error`, so it falls through to the generic `SearchClientError` fallback with `error_type: "search_error"` — exactly the opaque behavior this PR set out to improve. A quick `os.path.isfile` check or a startup-time `ValueError` similar to the existing `FLI_TIMEOUT` validation would surface the misconfiguration clearly.
How can I resolve this? If you propose a fix, please make it concise.1 participant
Summary
SearchCertificateErrorFLI_CA_BUNDLE,CURL_CA_BUNDLE, orREQUESTS_CA_BUNDLEerror_typevalues while preserving the legacyerrorand empty result fieldsTesting
uvx --system-certs ruff check .uvx --system-certs ruff format --check .UV_PROJECT_ENVIRONMENT=.venv-test uv --system-certs run pytest tests/search/test_client_errors.py tests/cli/test_errors.py tests/mcp/test_mcp_server_fixes.py -qGreptile Summary
This PR improves TLS error handling for the MCP server by introducing
SearchCertificateError, a new typed exception for certificate verification failures, and allowing users to configure a custom CA bundle viaFLI_CA_BUNDLE,CURL_CA_BUNDLE, orREQUESTS_CA_BUNDLE. It also refactors MCP error responses to include a stableerror_typefield alongside the existingerrorand result fields.SearchCertificateErroris added as a subclass ofSearchConnectionErrorand is classified before the parent class in everyisinstancechain, so the more-specific type is always matched first._wrap_request_errorusesgetattr(curl_exc, \"CertificateVerifyError\", ())as a forward-compatible guard, and_session()wires in the CA bundle when the env var is set._error_type/_error_responsehelpers centralize MCP error formatting, and the CLI, MCP server, and public__init__exports are all updated consistently.Confidence Score: 4/5
The change is safe to merge; all error classification paths are logically consistent and backed by new tests.
The exception hierarchy and isinstance ordering are correct throughout the CLI, MCP server, and _error_type helper. The two style concerns do not affect correctness in the normal path.
fli/search/client.py and fli/mcp/server.py are worth a second look for the CA bundle path handling and the error message prefix.
Important Files Changed
SearchCertificateErroras a subclass ofSearchConnectionError; clean and correctly placed in the hierarchy.getattrguard; minor concern that an invalid bundle path won't produce aSearchCertificateErrorat failure time._error_typeand_error_responsehelpers with correctisinstanceordering; the "Search failed: " prefix on typedSearchClientErrormessages is a minor style concern.SearchCertificateErrorhandling in the correct order (beforeSearchConnectionError) in both_friendly_messageandjson_error_payload.error_type: "certificate_error"in MCP flight and date search responses.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[HTTP POST to Google Flights] --> B{Exception?} B -->|No| C[Return response] B -->|Yes| D[_wrap_request_error] D --> E{Already SearchClientError?} E -->|Yes| F[Return as-is] E -->|No| G{CertificateVerifyError?} G -->|Yes| H[SearchCertificateError\ncertificate_error] G -->|No| I{curl_exc.Timeout?} I -->|Yes| J[SearchTimeoutError\ntimeout] I -->|No| K{curl_exc.ConnectionError?} K -->|Yes| L[SearchConnectionError\nconnection_error] K -->|No| M{curl_exc.HTTPError?} M -->|Yes| N[SearchHTTPError\nhttp_error] M -->|No| O[SearchClientError\nsearch_error] H & J & L & N & O --> P[_error_response] P --> Q[success: false, error: msg, error_type: ...]Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "Improve MCP TLS error reporting" | Re-trigger Greptile