potiuk commented on PR #70433:
URL: https://github.com/apache/airflow/pull/70433#issuecomment-5149053128
Thanks — the bug is real and well diagnosed. `_TYPE_MAP` maps `bytes` to
`{"type": "string"}` for the tool schema, nothing converted back, so a hook
method like `S3Hook.load_bytes` failed deep inside `BytesIO` with a `TypeError`
that gave no hint about where the bad value came from.
The implementation handles the cases I'd want: `Optional[bytes]` and `bytes
| None` both resolve, real `bytes` values pass through untouched, and the tests
fail without the fix since the fake hook reports the received type.
**One thing to settle before this goes in: what the string actually
contains.**
The schema advertises `{"type": "string"}` with no `contentEncoding`, so the
contract with the model is undefined. Models routinely emit base64 for binary
payloads. If one does that here, the base64 *text* gets UTF-8 encoded and
written to storage — wrong bytes, no error, upload reported as successful. That
swaps a loud crash for silent data corruption, which is the worse of the two.
Worth making the contract explicit rather than leaving it implied. Two
directions, and I don't think this should be decided in a review thread alone —
it sets the behaviour for every `bytes` parameter the toolset will ever expose:
1. Declare the encoding in the schema and follow it:
```python
bytes: {"type": "string", "contentEncoding": "base64"},
```
with a base64 decode in `call_tool`. This is the standard JSON Schema
mechanism, it supports genuinely binary payloads, and plain text sent by
mistake fails loudly on decode instead of silently storing the wrong thing.
2. Keep UTF-8 and say so in the schema, via a `description` on `bytes`-typed
parameters telling the model to send plain text. Smaller change, but it caps
these tools at text-only payloads.
Either is defensible; leaving it unstated is the option I'd avoid. Input
from whoever owns `common.ai` would be useful here, since this is a
provider-wide behaviour decision rather than a detail of this PR.
**Two smaller points:**
`except Exception` around `get_type_hints` is broader than the project
prefers — `NameError` (unresolvable forward reference) and `TypeError` are the
realistic failures. It matters more than usual here: the fallback to
`param.annotation` under `from __future__ import annotations` yields the
*string* `"bytes"`, which `_resolves_to_bytes` returns `False` for, so coercion
silently stops happening rather than failing. Worth narrowing the except and
leaving a comment about that fallback.
`_bytes_param_names` re-runs `inspect.signature` and `get_type_hints` on
every `call_tool`. `_build_json_schema_from_signature` already does both when
the tools are built, so the parameter names could be resolved once there and
reused per call.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
