ColtenOuO commented on PR #70433: URL: https://github.com/apache/airflow/pull/70433#issuecomment-5181159328
Thanks all — reworked and rebased onto current `main` (the branch predated #70096, which had rewritten the same region of `hook.py`). Title and description updated to match, since the change is no longer a UTF-8 coercion. Point by point: | Review point | How it was handled | | --- | --- | | Undefined string contract | `_TYPE_MAP[bytes]` now carries `contentEncoding: "base64"`; `call_tool` decodes with `b64decode(value, validate=True)` | | `contentEncoding` ignored by most APIs | The instruction is appended to each parameter's description, after the docstring enrichment loop that would otherwise overwrite it | | Document the contract | New "Binary parameters" section in `toolsets.rst` | | Keep decoding out of the args validator | Unchanged placement, with a comment at the site recording the `CachingToolset` fingerprint reason so it survives the next refactor | | `except Exception` too broad | Gone entirely, replaced by the per-parameter resolution below | | Per-parameter annotation resolution | `_resolve_annotations` substitutes unresolvable names with `Any` and retries, so one bad name no longer discards the whole signature | | Re-introspecting on every call | Resolved once in `get_tools` and stored per tool; no `lru_cache` on bound methods | | `VAR_POSITIONAL` / `VAR_KEYWORD` drift | The two passes are now one, and the skip happens before the `bytes` check, so the drift is structurally impossible | | Test duplication, untested union guard | The three duplicated cases are gone; a single parametrised test covers `bytes`, `bytes \| None`, `bytes \| str`, `list[bytes]`, `str` and an unannotated parameter | **One correction to what I said earlier in this thread.** I wrote that decode failures would be re-raised as `ValueError` so the model could retry. That was wrong, and @kaxil is right: pydantic-ai only turns `ModelRetry` (and `ValidationError` from the args-validation stage) into a retry prompt — a `ValueError` raised inside `call_tool` fails the run without the model ever seeing it. The code raises `ModelRetry`. The `UnicodeEncodeError` case in the same comment disappears with the UTF-8 encode itself. Two things that surfaced while implementing this: **A leak I introduced and caught before pushing.** Putting `contentEncoding` in `_TYPE_MAP` made it recurse into every union and list branch, so `bytes | str` and `list[bytes] | None` advertised base64 while `_resolves_to_bytes` — correctly — excluded them from decoding. `LambdaHook.invoke_lambda` and `LevelDBHook.run` both hit that shape, and a model trusting the schema would have handed the hook the encoded text: exactly the corruption this PR exists to prevent. `contentEncoding` is now applied in the same `if` that populates the decode set, and the parametrised test asserts both sides agree for every case. **The annotation fix reaches further than the `bytes` parameters.** `_build_json_schema_from_signature` shared the all-or-nothing `get_type_hints` call, so `CloudKMSHook.encrypt` was advertising *every* parameter as untyped, not just failing to decode `plaintext`. That is fixed by the same change. Deliberately left alone for now: - **The return direction.** Agreed it belongs here; I've replied with a concrete proposal in that thread rather than implementing ahead of the discussion, since it turns on a question the argument side doesn't have (return types can't be resolved statically — `GCSHook.download` is overloaded on `filename`). - **`list[bytes]`.** `LevelDBHook.run` decodes `key`/`value` but not `keys`/`values` in the same signature. Not a regression, and the parametrised test pins the current behaviour so it can't drift silently, but worth folding in if you'd rather the coercion recursed into containers. --- Drafted-by: Claude Code (Opus 5); reviewed by @ColtenOuO 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]
