Python: fix(core): include tool trajectory details in summarizer input - #8087
Open
JHf0912 wants to merge 4 commits into
Open
Python: fix(core): include tool trajectory details in summarizer input#8087JHf0912 wants to merge 4 commits into
JHf0912 wants to merge 4 commits into
Conversation
Introduce _format_summary_content as the dispatch point for per-content summary rendering and rewrite _format_summary_message to combine structured renderings with Message.text, keeping the legacy fallback to content types. Behavior is byte-identical for existing inputs; tool-call and tool-result branches follow in the next commit.
Render function_call contents with name, arguments, and call id, and function_result contents with result text, exception, and call id in _format_summary_content, so the summarizer LLM sees the tool trajectory instead of a bare content type. Text-only messages keep their legacy rendering byte-for-byte; messages mixing text and tool contents now include both.
Extend _format_summary_content with mcp_server_tool_call / mcp_server_tool_result (tool name, arguments, output, call id) and function_approval_request / function_approval_response (nested call name, approval id, decision) branches, completing the tool trajectory visible to the summarizer LLM. Fixes microsoft#8086
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A critical crash path and two moderate rendering defects remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds structured function, MCP, and approval trajectory details to Python summarizer transcripts.
Changes:
- Renders tool calls, results, exceptions, IDs, and approval decisions.
- Preserves text-only formatting.
- Adds focused regression tests.
Required fixes:
- Critical (1 vote): Safely stringify non-JSON-serializable MCP result mappings to prevent compaction crashes.
- Moderate (2 votes): Preserve chronological ordering of text and structured content.
- Moderate (1 vote): Use
tool_namewhen rendering approvals wrapping MCP calls.
File summaries
| File | Description |
|---|---|
python/packages/core/tests/core/test_compaction.py |
Adds formatter regression coverage. |
python/packages/core/agent_framework/_compaction.py |
Adds structured summary rendering; contains the unresolved findings above. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1232
to
+1235
| if content.type == "mcp_server_tool_result": | ||
| result_text = _tool_result_text(content.output) | ||
| if content.exception: | ||
| result_text = f"error({content.exception}): {result_text}" |
| return f"mcp_tool_result: {result_text}{call_id_suffix}" | ||
| if content.type in ("function_approval_request", "function_approval_response"): | ||
| nested_call = content.function_call | ||
| name = nested_call.name if nested_call is not None else "" |
Comment on lines
+1250
to
+1253
| parts = [_format_summary_content(content) for content in message.contents] | ||
| if message.text: | ||
| parts.append(message.text) | ||
| content_text = "; ".join(part for part in parts if part) |
- Stringify non-JSON-serializable MCP result mappings with the same json.dumps(default=str) fallback used by Content.from_function_result, preventing compaction crashes inside _select_summary_input_groups. - Preserve time order of text and structured contents in _format_summary_message by emitting consecutive text blocks in place. - Use tool_name as the fallback identity for MCP tool calls nested in approval contents. Addresses review feedback on PR microsoft#8087; adds three regression tests.
Author
|
Addressed the three findings in 6b5360d:
Added three regression tests (non-JSON MCP output, mixed-content ordering, MCP approval tool name); all 90 tests in \ ests/core/test_compaction.py\ pass, ruff clean. copilot-pull-request-reviewer please re-review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
SummarizationStrategyfeeds the summarizer LLM a transcript rendered by_format_summary_message(python/packages/core/agent_framework/_compaction.py), which only usesMessage.text. SinceMessage.text(python/packages/core/agent_framework/_types.py) concatenatesTextContentonly, tool rounds collapse to placeholders like2. [assistant] function_calland3. [tool] function_result. The summarizer therefore never sees the tool name, arguments, results, exceptions, orcall_id, and produces summaries that silently discard the tool trajectory.Fixes #8086
Solution
Add a per-content renderer
_format_summary_contentthat serializes tool trajectory contents into the summary input transcript, and route_format_summary_messagethrough it:function_call→function_call <name>(<arguments>) [call_id=<id>]function_result→function_result: <result> [call_id=<id>](witherror(<exception>)prefix when the call failed)mcp_server_tool_call/mcp_server_tool_result→ same shape with the MCP tool name and outputfunction_approval_request/function_approval_response→ nested call name, approval id, and decisionDesign decisions:
_compaction.pychange;SummarizationStrategy's trigger conditions, summary message shape, and trace links are untouched._select_summary_input_groupsstill selects whole groups by token budget; enriched groups simply carry their true payload, so the budget now reflects what the summarizer actually receives.Before → after (real output from the end-to-end reproduction):
Changes
python/packages/core/agent_framework/_compaction.py_format_summary_contentdispatch over tool-call / result / MCP / approval content types (reuses existing_tool_result_text)._format_summary_messagenow combines structured renderings withMessage.text, falling back to the legacy content-type list only when nothing else is available.python/packages/core/tests/core/test_compaction.pycall_id, mixed text/tool messages, text-only golden rendering, MCP tool details, approval request, approval response.Testing
SummarizationStrategyrun over a 6-message conversation with two tool rounds shows the summarizer input containing names, arguments, results, andcall_ids, and the summary message replacing the excluded originals with trace links intact.test_summarization_strategy_bounds_summary_input_to_complete_groups,test_summary_input_selection_does_not_retokenize_selected_transcript) pass unchanged; group-atomic selection semantics are preserved.Notes for Reviewer
include_tool_detailsopt-in/opt-out.text_reasoningrendering, structured JSON summary output, and no-call_idadjacency pairing ingroup_messagesare left unchanged; the last touches pairing semantics covered by specdocs/specs/004-python-function-calling-loop.mdand is proposed separately.before_run).