Re: [PR] Bug fixs: Coerce str back to bytes for bytes-typed HookToolset parameters [airflow]

["potiuk (via GitHub)" <gi...@apache.org>]

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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org