Add Frankfurter currency Omi integration app#7443
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9594419baa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if amount <= 0: | ||
| raise ValueError("amount must be greater than 0") |
There was a problem hiding this comment.
Reject NaN amounts as validation errors
_parse_amount only catches InvalidOperation while constructing Decimal, but Decimal("NaN") raises InvalidOperation later at if amount <= 0. That exception is not converted to ValueError, and convert_currency only handles ValueError/httpx.HTTPError, so a payload like {"amount":"NaN"} will produce a 500 instead of a normal tool error response. Please treat non-finite values as invalid input (or catch InvalidOperation for the comparison) so malformed user/LLM inputs fail gracefully.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR adds a new standalone Omi plugin (
Confidence Score: 3/5The app is self-contained and makes no writes, but invalid LLM inputs will surface as raw 422 errors rather than the ChatToolResponse the Omi platform expects, leaving tool calls broken for end users. When a Pydantic field validator rejects an input (e.g. a 4-letter currency code or a missing required field), FastAPI returns a {detail: [...]} 422 response instead of a ChatToolResponse. Every tool endpoint is affected and the mismatch will appear as a silent failure or crash from Omi's perspective. The remaining findings are non-blocking style and efficiency suggestions. plugins/omi-frankfurter-app/main.py — specifically the absence of a RequestValidationError exception handler and the per-request HTTP client lifecycle. Important Files Changed
Sequence DiagramsequenceDiagram
participant Omi as Omi Platform (LLM)
participant App as omi-frankfurter-app (FastAPI)
participant FX as api.frankfurter.app
Omi->>App: GET /.well-known/omi-tools.json
App-->>Omi: Tool manifest (3 tools)
Omi->>App: POST /tools/convert_currency
App->>App: Pydantic validation
alt Validation fails
App-->>Omi: HTTP 422 not ChatToolResponse
else Validation passes
App->>FX: "GET /latest?amount=X&from=Y&to=Z"
FX-->>App: base, date, rates
App-->>Omi: ChatToolResponse result
end
Omi->>App: POST /tools/get_latest_rates
App->>FX: "GET /latest?from=X&to=Y"
FX-->>App: base, date, rates
App-->>Omi: ChatToolResponse result
Omi->>App: POST /tools/list_supported_currencies
App->>FX: GET /currencies
FX-->>App: code name pairs
App-->>Omi: ChatToolResponse result
Reviews (1): Last reviewed commit: "Add Frankfurter currency Omi app" | Re-trigger Greptile |
| @app.post("/tools/convert_currency", response_model=ChatToolResponse) | ||
| async def convert_currency(request: ConvertCurrencyRequest) -> ChatToolResponse: | ||
| try: | ||
| amount = _parse_amount(request.amount) | ||
| payload = await _request_json( | ||
| "/latest", | ||
| { | ||
| "amount": str(amount), | ||
| "from": request.from_currency, | ||
| "to": ",".join(request.to_currencies), | ||
| }, | ||
| ) | ||
| rates = payload.get("rates") or {} | ||
| if not rates: | ||
| return ChatToolResponse(error="no rates returned for the requested currencies") | ||
|
|
||
| lines = [ | ||
| f"{_format_decimal(amount)} {payload.get('base', request.from_currency)} on {payload.get('date', 'latest')}:" | ||
| ] | ||
| for code in request.to_currencies: | ||
| if code in rates: | ||
| lines.append(f"- {code}: {_format_decimal(rates[code])}") | ||
| return ChatToolResponse(result="\n".join(lines)) | ||
| except (httpx.HTTPError, ValueError) as exc: | ||
| return ChatToolResponse(error=f"currency conversion failed: {exc}") |
There was a problem hiding this comment.
Pydantic validation errors return 422 instead of
ChatToolResponse
When the LLM (or a caller) sends data that fails the Pydantic validators — for example, a currency code that isn't exactly 3 alpha letters, a missing required field, or a to_currencies list that violates min_length — FastAPI raises a RequestValidationError and returns a {"detail": [...]} body with HTTP 422. All three tool endpoints are affected. The Omi platform likely expects ChatToolResponse on every response; an unhandled 422 will surface as a broken tool to the user.
A RequestValidationError exception handler registered on app would intercept these and return a properly shaped ChatToolResponse(error=...), keeping the response format consistent regardless of whether the error originates from input validation or from the downstream Frankfurter API call.
| async def _request_json(path: str, params: dict[str, Any] | None = None) -> dict[str, Any]: | ||
| async with httpx.AsyncClient(timeout=REQUEST_TIMEOUT_SECONDS) as client: | ||
| response = await client.get(f"{FRANKFURTER_BASE_URL}{path}", params=params) | ||
| response.raise_for_status() | ||
| return response.json() |
There was a problem hiding this comment.
New
httpx.AsyncClient created per request — A fresh client (including TLS handshake) is opened and torn down on every single Frankfurter call. Under any non-trivial load this is noticeably slower and prevents HTTP/2 multiplexing or keep-alive reuse. A module-level (or lifespan-scoped) shared client avoids this overhead.
| async def _request_json(path: str, params: dict[str, Any] | None = None) -> dict[str, Any]: | |
| async with httpx.AsyncClient(timeout=REQUEST_TIMEOUT_SECONDS) as client: | |
| response = await client.get(f"{FRANKFURTER_BASE_URL}{path}", params=params) | |
| response.raise_for_status() | |
| return response.json() | |
| _http_client: httpx.AsyncClient | None = None | |
| @app.on_event("startup") | |
| async def _startup() -> None: | |
| global _http_client | |
| _http_client = httpx.AsyncClient( | |
| base_url=FRANKFURTER_BASE_URL, | |
| timeout=REQUEST_TIMEOUT_SECONDS, | |
| ) | |
| @app.on_event("shutdown") | |
| async def _shutdown() -> None: | |
| if _http_client: | |
| await _http_client.aclose() | |
| async def _request_json(path: str, params: dict[str, Any] | None = None) -> dict[str, Any]: | |
| assert _http_client is not None, "HTTP client not initialised" | |
| response = await _http_client.get(path, params=params) | |
| response.raise_for_status() | |
| return response.json() |
|
|
||
|
|
||
| class ConvertCurrencyRequest(BaseModel): | ||
| amount: str | float | int = Field(..., description="Amount to convert, such as 50 or '19.95'.") |
There was a problem hiding this comment.
Manifest
amount type is number but the Pydantic model accepts str | float | int — The JSON Schema in the tool manifest advertises "type": "number", so the Omi LLM will always send a JSON number. The str branch in the union is therefore dead in practice, and the mismatch can confuse developers reading either side. Aligning the manifest and the model (both number) removes the ambiguity without changing runtime behaviour.
| amount: str | float | int = Field(..., description="Amount to convert, such as 50 or '19.95'.") | |
| amount: float | int = Field(..., description="Amount to convert, such as 50 or 19.95.", gt=0) |
|
Updated after the automated review note:
Verification rerun: |
Summary
Adds a standalone Frankfurter currency integration app for Omi under
plugins/omi-frankfurter-app.The app exposes three no-auth chat tools:
convert_currencyfor converting an amount into one or more target currenciesget_latest_ratesfor latest reference rates from a base currencylist_supported_currenciesfor supported Frankfurter currency codesNo OAuth, accounts, API keys, or environment variables are required.
Related to #3120 and the integration-app bounty discussion. If maintainers consider this eligible for one of the integration-app bounties, I can follow the documented payout process after merge.
Verification
python3 -m py_compile omi-frankfurter-app/plugins/omi-frankfurter-app/main.py50 USD -> EUR,CNY