mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-10-26 13:16:24 +00:00
fix(middleware): preserve message fields when PII redaction rebuilds a user message (#5968)
* fix(middleware): preserve message fields when PII redaction rebuilds a user message PiiRedactionMiddleware._process_request rebuilt the redacted HumanMessage with a hand-constructed constructor call copying only content, id, name and additional_kwargs, silently dropping response_metadata and every other field of the original. That is the third and last site of the family already fixed for ThreadDataMiddleware (#5823) and InputSanitizationMiddleware (#5875); the review on #5823 scoped the follow-up to the two input-sanitization sites and left this one out. The middleware was also inconsistent with itself: _redact_tool_message in the same file already rebuilds with model_copy and carries the comment explaining that a hand-built constructor call silently drops response_metadata. The user message now rebuilds with msg.model_copy(update={...}). The content is deep-copied because _redact_content appends untouched content blocks by reference: without the copy, mutating a non-text block on the redacted message would write through to the message retained in thread state. Red/green: test_redacted_user_message_preserves_other_fields asserts response_metadata/id/name/additional_kwargs survive the rewrite — red on main (response_metadata == {}), green here. A second test guards the aliasing behaviour above and goes red if the deep copy is dropped. Both suites 258 passed; ruff 0.15.12 check + format clean repo-wide. Fixes #5967 Signed-off-by: yetuge <2219677952@qq.com> * fix(middleware): keep the PII rewrite on copy failure and detach metadata Addresses the two copy-boundary findings on the previous commit. 1. A content block may carry a value whose copy raises. deepcopy(content) then raised out of _process_request, _try_process fell back to the original request, and the handler received raw PII unredacted. Copying now falls back to a shallow per-container copy, so a copy failure still produces a redacted message and never bypasses redaction. 2. msg.model_copy(update=...) is shallow, so the rebuilt message and the original shared one response_metadata dict: mutating metadata on the model-facing message wrote through to the request message retained in thread state. response_metadata is now copied independently. Red/green: test_preserved_metadata_is_not_shared_with_the_original and test_redaction_survives_a_content_block_that_cannot_be_copied both fail on the reviewed commit (the second with the raw address reaching the handler) and pass here. Suites: pii 77 passed, and pii + input_sanitization + uploads 327 passed. ruff check + format --check clean repo-wide (1694 files). Also drops the "third and last site" wording from the previous commit message's claim: UploadsMiddleware is a further site of the same pattern (#5972). Signed-off-by: yetuge <2219677952@qq.com> --------- Signed-off-by: yetuge <2219677952@qq.com>
This commit is contained in:
parent
c89ba45f7f
commit
34a105c084
@ -55,6 +55,7 @@ import hmac
|
||||
import logging
|
||||
import re
|
||||
from collections.abc import Awaitable, Callable, Sequence
|
||||
from copy import deepcopy
|
||||
from dataclasses import dataclass
|
||||
from dataclasses import replace as dc_replace
|
||||
from typing import override
|
||||
@ -291,6 +292,31 @@ class _Redactor:
|
||||
return replace
|
||||
|
||||
|
||||
def _independent_copy(value: object, *, what: str) -> object:
|
||||
"""Copy *value* so the rebuilt message shares no mutable state with the original.
|
||||
|
||||
``deepcopy`` keeps nested mutable blocks and metadata independent, which is
|
||||
what the rewrite needs: ``_redact_content`` retains references to the blocks
|
||||
it leaves untouched, and ``model_copy`` alone is shallow. It can raise for an
|
||||
exotic value a caller placed in a block, and a copy failure must never skip
|
||||
the rewrite — that would hand raw PII to the model. Fall back to a shallow
|
||||
per-container copy, which still detaches the containers the rewrite touches.
|
||||
"""
|
||||
try:
|
||||
return deepcopy(value)
|
||||
except Exception:
|
||||
logger.warning(
|
||||
"PII redaction could not deep-copy %s; using a shallow copy",
|
||||
what,
|
||||
exc_info=True,
|
||||
)
|
||||
if isinstance(value, list):
|
||||
return [dict(item) if isinstance(item, dict) else item for item in value]
|
||||
if isinstance(value, dict):
|
||||
return dict(value)
|
||||
return value
|
||||
|
||||
|
||||
def _redact_content(content: object, redactor: _Redactor) -> tuple[object, bool]:
|
||||
"""Redact *content*, preserving its shape. Returns ``(content, changed)``.
|
||||
|
||||
@ -370,11 +396,14 @@ class PiiRedactionMiddleware(AgentMiddleware[AgentState]):
|
||||
continue
|
||||
if not changed_msg:
|
||||
continue
|
||||
messages[index] = HumanMessage(
|
||||
content=content,
|
||||
id=msg.id,
|
||||
name=msg.name,
|
||||
additional_kwargs=dict(msg.additional_kwargs or {}),
|
||||
messages[index] = msg.model_copy(
|
||||
update={
|
||||
"content": _independent_copy(content, what="message content"),
|
||||
"additional_kwargs": dict(msg.additional_kwargs or {}),
|
||||
# model_copy is shallow, so the preserved metadata would
|
||||
# otherwise be the same dict as the original message's.
|
||||
"response_metadata": _independent_copy(dict(msg.response_metadata or {}), what="response_metadata"),
|
||||
},
|
||||
)
|
||||
changed = True
|
||||
updates = {}
|
||||
|
||||
@ -248,6 +248,55 @@ class TestModelCallBoundary:
|
||||
messages, _ = _run_model_call(_make_middleware(), [original])
|
||||
assert messages[0].additional_kwargs["custom"] == "v"
|
||||
|
||||
def test_redacted_user_message_preserves_other_fields(self):
|
||||
# The rewrite must not drop fields it does not touch: a hand-built
|
||||
# constructor call silently loses response_metadata and every other
|
||||
# field outside content/id/name/additional_kwargs. The tool-result
|
||||
# seam in this same middleware already rebuilds with model_copy for
|
||||
# exactly that reason.
|
||||
original = HumanMessage(
|
||||
content="alice@example.com",
|
||||
id="msg-1",
|
||||
name="user",
|
||||
additional_kwargs={"custom": "v"},
|
||||
response_metadata={"source": "gateway"},
|
||||
)
|
||||
messages, _ = _run_model_call(_make_middleware(), [original])
|
||||
assert messages[0].content == EMAIL_ALICE
|
||||
assert messages[0].id == "msg-1"
|
||||
assert messages[0].name == "user"
|
||||
assert messages[0].additional_kwargs["custom"] == "v"
|
||||
assert messages[0].response_metadata == {"source": "gateway"}
|
||||
|
||||
def test_preserved_metadata_is_not_shared_with_the_original(self):
|
||||
# model_copy is shallow: without an explicit copy the rewritten message
|
||||
# and the original would hold the same response_metadata dict, so
|
||||
# writing metadata on the model-facing message would mutate the
|
||||
# request message retained in thread state.
|
||||
original = HumanMessage(
|
||||
content="alice@example.com",
|
||||
response_metadata={"source": "gateway", "nested": {"k": "v"}},
|
||||
)
|
||||
messages, request = _run_model_call(_make_middleware(), [original])
|
||||
assert messages[0].response_metadata == original.response_metadata
|
||||
|
||||
messages[0].response_metadata["source"] = "mutated"
|
||||
assert request.messages[0].response_metadata["source"] == "gateway"
|
||||
|
||||
def test_redaction_survives_a_content_block_that_cannot_be_copied(self):
|
||||
# A caller can put a value whose copy raises into a block. The rewrite
|
||||
# must still happen: letting the copy failure escape would fail the
|
||||
# whole request open and hand raw PII to the model.
|
||||
class NoCopy:
|
||||
def __deepcopy__(self, memo):
|
||||
raise RuntimeError("cannot copy this block value")
|
||||
|
||||
original = HumanMessage(content=[{"type": "text", "text": "reach me at alice@example.com", "meta": NoCopy()}])
|
||||
messages, _ = _run_model_call(_make_middleware(), [original])
|
||||
|
||||
assert EMAIL_ALICE in messages[0].content[0]["text"]
|
||||
assert "alice@example.com" not in messages[0].content[0]["text"]
|
||||
|
||||
def test_ai_message_untouched(self):
|
||||
ai = AIMessage("contact alice@example.com")
|
||||
messages, _ = _run_model_call(_make_middleware(), [ai])
|
||||
@ -300,6 +349,17 @@ class TestModelCallBoundary:
|
||||
# The original message object is untouched.
|
||||
assert original.content[0] == "reach me at alice@example.com"
|
||||
|
||||
def test_rebuilt_user_message_does_not_alias_the_original_content(self):
|
||||
# _redact_content appends untouched blocks by reference, so the
|
||||
# rebuilt message must copy the container: without the copy, mutating
|
||||
# a non-text block on the redacted message would write through to the
|
||||
# message kept in thread state.
|
||||
image_block = {"type": "image_url", "image_url": {"url": "https://example.com/a.png"}}
|
||||
original = HumanMessage(["alice@example.com", image_block])
|
||||
messages, _ = _run_model_call(_make_middleware(), [original])
|
||||
messages[0].content[1]["image_url"]["url"] = "https://example.com/mutated.png"
|
||||
assert original.content[1]["image_url"]["url"] == "https://example.com/a.png"
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Tool boundary
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user