kaxil commented on code in PR #70830:
URL: https://github.com/apache/airflow/pull/70830#discussion_r3720123766
##########
providers/common/ai/src/airflow/providers/common/ai/policies/retry.py:
##########
@@ -134,14 +143,19 @@ def __init__(
fallback_rules: list[RetryRule] | None = None,
timeout: float = 30.0,
*,
- redact_exception: bool = True,
+ # redact() is typed for arbitrary containers; a str in always yields a
str out.
+ redactor: Callable[[str], str] | None = lambda message: cast("str",
redact(message)),
Review Comment:
`None` here turns redaction off, but for the three params just above
(`model_id`, `instructions`, `fallback_rules`) `None` means "use the default".
Anyone assembling kwargs programmatically, say `redactor=cfg.get("redactor")`,
silently ships raw exception text to the provider whenever that key is absent,
and neither the signature nor the params table makes that visible. Could `None`
keep the default masker, with the opt-out spelled out separately
(`redactor=str` documented as the no-op, or a `redact_exception=False`-style
flag)?
##########
providers/common/ai/tests/unit/common/ai/policies/test_retry.py:
##########
@@ -154,6 +154,85 @@ def
test_prompt_keeps_raw_message_when_redaction_disabled(self, mock_hook_cls):
prompt = mock_agent.run_sync.call_args[0][0]
assert secret_value in prompt
+ @pytest.mark.enable_redact
+ @patch("airflow.providers.common.ai.hooks.pydantic_ai.PydanticAIHook",
autospec=True)
+ def test_custom_redactor_replaces_masker_instead_of_stacking(self,
mock_hook_cls):
+ """A custom redactor replaces the secrets masker entirely -- it is not
applied on top."""
+ secret_value = "super-secret-conn-password"
+ mask_secret(secret_value)
Review Comment:
This test and `test_truncation_happens_after_redaction` call `mask_secret()`
without the `reset_secrets_masker()` that the two redaction tests above use, so
they inherit patterns from earlier tests and leave this secret registered for
whatever runs next. Same call at the top would keep them consistent with the
siblings.
##########
providers/common/ai/docs/retry_policies.rst:
##########
@@ -158,22 +159,66 @@ Parameters
* - ``timeout``
- 30.0
- Max seconds to wait for the LLM response before falling back.
- * - ``redact_exception``
- - True
- - When ``True``, the exception's string representation is passed through
- Airflow's secrets masker before being added to the classification
- prompt. This only masks values already registered via
- ``mask_secret()`` (e.g. connection passwords Airflow captured while
- resolving the failing task's connections) -- it is not general-purpose
- PII detection and will not catch arbitrary sensitive strings that were
- never registered as secrets. Set to ``False`` only if you are certain
- your exception messages contain no sensitive data and you need the
- raw text for accurate classification.
+ * - ``redactor``
+ - Airflow's secrets masker
+ - Callable ``(str) -> str`` applied to the exception's string
+ representation before it is added to the classification prompt. The
+ default only masks values already registered via ``mask_secret()``
+ (e.g. connection passwords Airflow captured while resolving the
+ failing task's connections) -- it is not general-purpose PII
+ detection and will not catch arbitrary sensitive strings that were
+ never registered as secrets. Passing a custom callable **replaces**
+ the default masker entirely rather than stacking on top of it; pass
+ ``None`` to disable redaction altogether.
+ * - ``max_exception_length``
+ - 4096
+ - Maximum number of characters of the (already redacted) exception
+ message included in the prompt. Longer messages are truncated with a
+ trailing ``"... (truncated)"`` marker. Must be a positive integer.
+
+Custom redactors
+----------------
+
+The default ``redactor`` only masks values already registered with Airflow's
+secrets masker via ``mask_secret()``. It does not detect free-text PII --
+email addresses, customer names, account numbers -- that were never
+registered as secrets. If your task's exception messages can contain that
+kind of data, supply your own ``redactor`` callable. It **replaces** the
+default masker rather than running in addition to it, so combine your own
+logic with :func:`~airflow.sdk.log.redact` yourself if you still want
+known-secret masking too:
+
+.. code-block:: python
+
+ import re
+
+ from airflow.sdk.log import redact
Review Comment:
`redact` isn't part of the Task SDK's documented public surface:
`task-sdk/docs/api.rst` autodocs `airflow.sdk.log.mask_secret` but not
`redact`, which is only a re-export from `airflow.sdk._shared.secrets_masker`.
Its home has moved four times, which is why `common.compat.sdk` carries a
fallback chain for it (`airflow.sdk.log`, `airflow.sdk._shared.secrets_masker`,
`airflow.sdk.execution_time.secrets_masker`,
`airflow.utils.log.secrets_masker`) and why this module imports it through the
shim rather than directly. Pointing DAG authors at the raw path means their
redactor breaks on the next move. Would naming the default lambda as a
module-level `default_redactor` and exporting it work better, so the docs can
say `from airflow.providers.common.ai.policies.retry import default_redactor`?
--
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]
