Uh oh!
There was an error while loading. Please reload this page.
Feat/mcp structured tool results - #135
Conversation
Add an immutable, versioned tool-result contract and make execution claims safe for concurrent replay. Preserve legacy text records while preventing terminal ledger overwrites.
Normalize MCP and local tool outputs once at the provider boundary, expose structured values to trusted hooks, and keep model messages and logs text-only. Cover structured-only, malformed, replay, concurrency, and HITL behavior.
Document normalization ownership, model visibility, artifact bounds, hook compatibility, and concurrency-safe persisted replay in ADR 0004 and runtime guides.
AmitAvital1
left a comment
There was a problem hiding this comment.
Requesting changes for correctness and contract issues found in the structured-result execution path; details are inline.
| await self._publish_processing_failure(call, "Tool error processing failed") | ||
| raise | ||
| result = NormalizedToolResult.text_only(model_text or f"Tool error: {exc}") | ||
| await self._execution_manager.finish_execution(call.exec_id, status="failed", result=result) |
There was a problem hiding this comment.
Cancellation while awaiting this write leaves the claim at started: _record_error() is outside the recovery wrapper used by _record_success().
Please shield/finalize it and add a cancellation test; otherwise duplicate callers can wait forever.
| try: | ||
| if isinstance(result, ToolMessage): | ||
| model_text, content_structured = _tool_message_content(result.content) | ||
| artifact_structured, artifact = _split_artifact(result.artifact) |
There was a problem hiding this comment.
This branch relies on the artifact contract introduced by langchain-mcp-adapters 0.2.0, but pyproject.toml still permits 0.1.x.
Could we bump the minimum version? Otherwise valid installations silently lose structuredContent, and older artifact shapes can fail normalization.
| object.__setattr__( | ||
| self, | ||
| "_structured_json", | ||
| _canonical_json(structured, field_name="structured result") |
There was a problem hiding this comment.
structured is canonicalized and later decoded, copied, and persisted with only a depth check; unlike artifact, there is no item or byte budget.
A large MCP response can multiply memory use across normalization, hooks, and replay—please reuse a bounded-JSON policy here.
| from agent_engine.runtime.tool_models import ToolProviderName | ||
| from agent_engine.runtime.tool_results import PersistedToolResult | ||
| ToolExecutionStatus = Literal["started", "succeeded", "failed"] |
There was a problem hiding this comment.
Could this be a StrEnum, consistent with RunStatus and ApprovalStatus below?
The raw values are repeated in the repository, manager, and invoker; enum members would remove these magic state strings.
Make terminal execution writes cancellation-safe, model execution states with a StrEnum, bound structured JSON payloads, and require the MCP adapter version that provides the structured-content artifact contract. Add regressions for cancellation with duplicate waiters and oversized MCP structured results.
Record the structured JSON budgets, execution status enum, cancellation-safe ledger expectations, and langchain-mcp-adapters 0.2 minimum.
Asaf-prog
commented
Sep 2, 2026
Thanks for the detailed review — I addressed all four findings:
|
Summary
Preserve structured MCP tool results throughout Extra's execution pipeline instead of collapsing every successful tool result into plain text.
This builds on #133, which fixed MCP text extraction, and adds a provider-agnostic normalized tool-result model that keeps:
The structured result now survives hooks, persistence, idempotent replay, and HITL resume without leaking provider-specific LangChain objects into the core runtime.
What changed
structuredContentinstead of dropping it.dict/listtool results where applicable.ToolMessage.transform_tool_resulthooks compatible while preserving structured data.Runtime flow
Provider / LangChain result
↓
Tool-result normalization
↓
NormalizedToolResult
├── text
├── structured
└── bounded artifact metadata
↓
Hooks
↓
Execution ledger
↓
Replay / HITL resume
text only
↓
Model conversation
Backwards compatibility
Tests
Added/updated coverage for:
structuredContent;Validation
git diff --checkpassed.Out of scope