fix: filter TOOL_CALL and TOOL_RESPONSE messages from managed-agent summary#2565
fix: filter TOOL_CALL and TOOL_RESPONSE messages from managed-agent summary#2565rkfshakti wants to merge 2 commits into
Conversation
gnanirahulnutakki
left a comment
There was a problem hiding this comment.
One scope-blocking finding. AI-assisted review; I verified it against the PR's current two-commit diff and the separate #2564 branch.
There was a problem hiding this comment.
Could you remove this file/commit from this PR? The branch currently includes commit 874a6f7 and 67 lines of big-integer executor changes from #2564 / #2473, while this PR is scoped to #2424 and the managed-agent summary. As-is, merging would couple unrelated runtime behavior, duplicate #2564, and make the stated five-line/test scope inaccurate. Rebasing and cherry-picking 2deb0a6 onto current main should isolate the intended fix.
There was a problem hiding this comment.
please review ; i have made the changes
…ummary When a MultiStepAgent is used as a managed sub-agent with provide_run_summary=True, the parent's tool observation contains the sub-agent's entire internal message history — including TOOL_CALL and TOOL_RESPONSE messages that leak raw tool arguments, observations, and potentially PII/secrets into the parent agent's context. summary_mode=True in ActionStep.to_messages() only suppresses the assistant's model_output text. TOOL_CALL and TOOL_RESPONSE messages are still emitted, so the summary still contains 'Calling tools:' and 'Observation:' entries with full tool I/O. The fix filters out TOOL_CALL and TOOL_RESPONSE messages at the ManagedAgent.__call__ boundary before appending to the summary. Only ASSISTANT, USER, and SYSTEM messages are included, which contain the final answer and high-level task information without raw tool internals. Fixes huggingface#2424
2deb0a6 to
ebd4180
Compare
|
@gnanirahulnutakki — thanks for catching that. The branch was based on #2564's commit by mistake. I've rebased to drop the unrelated big-integer changes — the PR now contains only the managed-agent summary fix (single commit on current |
gnanirahulnutakki
left a comment
There was a problem hiding this comment.
The unrelated commit is gone; exact head ebd418003d14 is now one commit and one source file. The existing managed-agent summary test cases and Ruff pass locally. One remaining regression-coverage gap is noted inline. Full extras are not installable on this macOS host because optional Linux-only bashkit/NVIDIA wheels are unavailable, so I validated with the repository's minimal editable test setup.
| # Skip tool-call and tool-response messages: they contain raw | ||
| # tool arguments, observations, and potentially PII/secrets | ||
| # that should not leak into the parent agent's context. | ||
| if message.role in (MessageRole.TOOL_CALL, MessageRole.TOOL_RESPONSE): |
There was a problem hiding this comment.
[P2] Add regression coverage for the filtered roles
The existing test_call_with_provide_run_summary only supplies an ASSISTANT message, so it passes even if this condition is removed or later regresses. Because the fix is specifically meant to prevent raw tool arguments/outputs (and potentially secrets) from entering the parent context, please extend that test to return ASSISTANT, TOOL_CALL, and TOOL_RESPONSE messages, then assert the assistant summary remains while the raw tool-call and tool-response payloads are absent.
…filtering (huggingface#2424) The existing test_call_with_provide_run_summary only supplied an ASSISTANT message, so it passed even if the TOOL_CALL/TOOL_RESPONSE filter were removed. Extend coverage with a case that returns ASSISTANT, TOOL_CALL, and TOOL_RESPONSE messages and asserts the raw tool arguments/observations (which may contain secrets) are excluded from the parent agent's context while assistant summaries remain.
|
Thanks for the regression-coverage note — agreed the old test only carried an ASSISTANT message and would have stayed green if the filter regressed. I added |
gnanirahulnutakki
left a comment
There was a problem hiding this comment.
Re-reviewed on e5685e0. The new regression feeds the managed-agent summary both assistant messages and secret-like TOOL_CALL/TOOL_RESPONSE payloads, then verifies the assistant summaries remain while the raw tool data is absent. Locally, all three focused provide_run_summary tests passed, and Ruff check plus format verification passed on both changed files. This addresses the regression-coverage finding.
Problem
When a
MultiStepAgentis used as a managed sub-agent withprovide_run_summary=True, the parent's tool observation contains the sub-agent's entire internal message history — includingTOOL_CALLandTOOL_RESPONSEmessages that leak raw tool arguments, observations, and potentially PII/secrets into the parent agent's context.Root cause
summary_mode=TrueinActionStep.to_messages()only suppresses the assistant'smodel_outputtext.TOOL_CALLandTOOL_RESPONSEmessages are still emitted, so the summary still contains"Calling tools:"and"Observation:"entries with full tool I/O.Fix
Filter out
TOOL_CALLandTOOL_RESPONSEmessages at theManagedAgent.__call__boundary before appending to the summary. OnlyASSISTANT,USER, andSYSTEMmessages are included, which contain the final answer and high-level task information without raw tool internals.Testing
Fixes #2424