Python: fix(core): include tool trajectory details in summarizer input - #8087
Python: fix(core): include tool trajectory details in summarizer input#8087JHf0912 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
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.
| 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 "" |
| 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.
|
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. |
Ricky-7-Yan
left a comment
There was a problem hiding this comment.
Verified the current 6b5360da head independently against the exact two-file diff:
- the complete
tests/core/test_compaction.pysuite passes; - Ruff check and format check pass for both changed files;
- the three earlier findings are resolved: mapping outputs now use the established
default=strfallback, mixed text/tool contents retain their original order, and MCP approvals usetool_namewhennameis absent.
I also checked compatibility with the existing Message.text behavior for text-only, consecutive-text, text-plus-unrendered-content, and non-text fallback cases. No new blocking issue found.
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).