fix(converter): keep only last message from full-history agent output (HYBIM-988) - #227
fix(converter): keep only last message from full-history agent output (HYBIM-988)#227etserend wants to merge 12 commits into
Conversation
fercor-cisco
left a comment
There was a problem hiding this comment.
🤖 This review was generated by the Astra agent (claude-opus-5). It may contain mistakes.
Verdict: request_changes — The reduction is gated on full_history alone rather than on the output actually being accumulated history, so it silently drops real messages from non-root LangGraph node spans (e.g. a ToolNode returning one ToolMessage per parallel tool call).
General Comments
- 🟠 major (design): The fix is applied at the wrong layer, and a narrower gate exists.
_set_orchestration_content is shared by set_workflow_attributes and set_agent_attributes, but the bug in HYBIM-988 is specific to the root invoke_agent Agent span, where LangGraph hands back the whole accumulated state. Gating on full_history alone catches every span whose output happens to be a top-level {"messages": [...]} container — which via on_chain_end (handlers/langchain/handler.py:119-125) is every LangGraph node, since base_handler.py:145-157 maps non-root chains to WorkflowSpans and serialization.py:197/245/257 guarantees each serialized message carries a role.
Note there is already precedent for solving this at the handler layer: handlers/langchain/middleware.py:199-202 and :213-216 (after_agent / aafter_agent) already do exactly this keep-last reduction on the root agent node before it reaches the converter. Two reasonable directions:
- Gate on the dedup having fired (smallest change, keeps the converter as the single place). Per the ticket, dedup does fire in the reported scenario, so this still fixes the bug while leaving node-level spans alone. See the line comment.
- Do it in the callback handler, mirroring the middleware, so only the root agent node is reduced and the converter stays a faithful mapper.
Option 1 is the smaller diff; option 2 is more consistent with the existing middleware behavior and avoids the converter making root-vs-node judgements it has no information to make. Either is preferable to the current unconditional gate.
- 🟡 minor (documentation): No
CHANGELOG.mdentry under[Unreleased]. This changes what the SDK puts on the wire ingen_ai.output.messages/splunk_ao.output.messages, which is user-visible behavior, andAGENTS.md(Change Workflow step 3) requires a changelog update in that case. The closest precedent, #215 (fix(converter): emit OTel multimodal message parts), did add one.
Follow-ups
Suggested follow-up work that could be tracked as Jira tickets:
src/splunk_ao/converter/attribute_mapping.py:220-232:full_historyis only True for a top-level"messages"key (container is source, line 228). Accumulated history arriving via a nested"update"key — i.e. a LangGraphCommand, whichserialization.py:280-286does serialize as{"update": {...}}— or as a bare JSON list is classifiedfull_history=Falseand so is never deduped or reduced. Worth confirming whether that asymmetry is intentional; if a root agent output can ever arrive inCommandform, HYBIM-988 would still reproduce there.src/splunk_ao/handlers/langchain/middleware.py:196-218:after_agent/aafter_agentalready reduce the root agent state to its last message at the handler layer. Once this PR adds an equivalent reduction in the converter, two layers implement the same policy independently. Consider consolidating on one so the behavior cannot drift between the middleware and callback-handler paths.
fercor-cisco
left a comment
There was a problem hiding this comment.
🤖 This review was generated by the Astra agent (claude-opus-5). It may contain mistakes.
Verdict: request_changes — The new terminal-assistant trim emits an empty gen_ai.output.messages array (total output loss) whenever the post-dedup remainder does not end with an assistant message — verified by running the converter.
General Comments
- 🟡 minor (documentation): The PR description (and the ticket's proposed fix) still say "keep only the last message" /
output_messages = [output_messages[-1]], but the implementation now keeps the whole trailing run of assistant messages. That divergence is the interesting design decision in this PR and it isn't explained anywhere — please update the description with the rationale so the next reader doesn't assume the ticket snippet is what landed.
Related question: per the ticket, the UI renders the first message's text content. If the trailing assistant run ever has more than one entry and the first of them has empty text (e.g. a tool-call AIMessage immediately followed by another AIMessage, as LangGraph's structured-response node can produce), the original — symptom returns. Did you confirm the UI picks the last/non-empty message, or is keeping >1 terminal message safe only because that shape doesn't occur in practice?
Follow-ups
Suggested follow-up work that could be tracked as Jira tickets:
src/splunk_ao/handlers/langchain/middleware.py:196-218:after_agent/aafter_agentreduce the root agent state to its last message at the handler layer, while_set_orchestration_contentnow performs a similar (but not identical — trailing assistant run vs. strictly last message) reduction in the converter. Two layers implement overlapping policy with different semantics; consolidating on one would prevent drift. (Also raised in the earlier review.)src/splunk_ao/converter/attribute_mapping.py:222-232:full_historyis only True whenmessagessits at the top level (container is source). Accumulated history arriving via a nestedupdatekey (a LangGraphCommand, whichutils/serialization.pydoes serialize as{"update": {...}}) or as a bare JSON list is classifiedfull_history=False, so it is never deduped or reduced. Worth confirming whether a root agent output can arrive in that form; if so, HYBIM-988 still reproduces there.
f69cd41 to
ce3d54c
Compare
… and add WorkflowSpan trim test Removes the tool_call finish-reason inference added to _with_finish_reasons — it is unrelated to HYBIM-988 and widens blast radius to LLM spans. Updates CHANGELOG and adds WorkflowSpan coverage for the full-history trim path. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…tation Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
fercor-cisco
left a comment
There was a problem hiding this comment.
🤖 This review was generated by the Astra agent (claude-opus-5). It may contain mistakes.
Verdict: approve — The dedup-gated keep-last trim is correct, non-vacuously tested, lint-clean, and changelogged; remaining concerns are minor edge cases and a documentation/scoping question that don't block merge.
General Comments
- 🟡 minor (question): Design question: should the SDK drop data from the wire to accommodate a UI rendering rule?
The root cause in HYBIM-988 is that the UI renders the first entry of gen_ai.output.messages. The fix here responds by emitting only one message, which means agent/workflow spans permanently lose the intermediate tool-call and tool-response messages from their output attribute — that data is not recoverable downstream by any other consumer of the OTLP stream (dashboards, raw span queries, third-party backends).
Mitigating factor: those intermediate messages generally also appear on the child LLM/tool spans, so the trace as a whole is not lossy. That's probably why this is acceptable. But it's worth stating the trade-off explicitly somewhere, and confirming a UI-side fix (render the last, or the last non-empty, message) was considered and rejected — otherwise this reduction becomes permanent wire behavior that a future UI change cannot undo.
Also note the reduction is not LangGraph-specific: _set_orchestration_content serves every WorkflowSpan/AgentSpan, including hand-instrumented ones created via the object API or the @workflow/@agent decorators. Any caller that logs input={"messages": [...]} and output={"messages": [...same prefix..., a, b]} now sees only b on the wire. The CHANGELOG entry covers this at a high level ("full-history spans"), which is probably enough, but the blast radius is wider than the LangGraph callback path the code comment describes.
Follow-ups
Suggested follow-up work that could be tracked as Jira tickets:
src/splunk_ao/handlers/langchain/middleware.py:196-218:after_agent/aafter_agentreduce the root agent state to its last message at the handler layer, while_set_orchestration_contentnow performs the same reduction in the converter behind a prefix-match gate. Two layers implement overlapping policy with different trigger conditions, so the observable output for the same application differs depending on which instrumentation path is active. Consolidating on one layer (converter-only, with the handler passing state through) would remove the divergence. Raised in both prior reviews and still unaddressed.src/splunk_ao/converter/attribute_mapping.py:217-239:_message_containeronly reportsfull_history=Truewhenmessagessits at the top level (container is source, line 228). Accumulated history arriving through a nestedupdatekey — the LangGraphCommandshape thatutils/serialization.py:280-286does produce — is classifiedfull_history=Falseand therefore never deduped or trimmed. Verified: aWorkflowSpanwithoutput={"update": {"messages": [user, tool_call_ai, tool_msg, final_ai]}}and a matching input prefix emits all four messages. Worth confirming whether a root agent output can ever arrive inCommandform; if so, HYBIM-988 reproduces there unchanged.
…im on history_stripped - _with_finish_reasons: infer "tool_call" finish reason when a message has tool_call parts but no explicit source finish reason; "unknown" remains the fallback for all other cases - _set_orchestration_content: split dedup and [-1:] trim using a history_stripped flag so parallel tool-call outputs (ToolNode) are never collapsed to one message when the output does not echo the input prefix - Update two test assertions from "unknown" to "tool_call" for tool-call assistant message cases - CHANGELOG updated with both fixes
…d drop redundant test
fercor-cisco
left a comment
There was a problem hiding this comment.
🤖 This review was generated by the Astra agent (claude-opus-5). It may contain mistakes.
Verdict: approve — The dedup-gated keep-last trim is correct for the targeted LangGraph shape, non-vacuously tested (54 tests pass), and changelogged; remaining items are a role-blind trim edge case, missing LlmSpan coverage for the bundled finish-reason change, and formatting/naming nits.
General Comments
- 🟡 minor (testing): The bundled
_with_finish_reasonsinference changes LLM spans too, but all four new finish-reason tests useWorkflowSpan.
_with_finish_reasons is also called from _output_messages (attribute_mapping.py:286), which serves set_llm_attributes (:368). Verified locally: an LlmSpan with finish_reason=None whose output carries tool_calls now emits finish_reason="tool_call" where origin/main emitted "unknown" — and no test in tests/test_attribute_mapping.py pins that. The closest existing test (test_llm_messages_preserve_tool_calls_and_tool_responses, line 84) sets finish_reason="stop" and only asserts on gen_ai.input.messages, so the LLM-span half of this change is currently unguarded.
Please add an LlmSpan case (output with tool calls, finish_reason=None → "tool_call"), since the CHANGELOG entry advertises this for gen_ai.output.messages generally, not just orchestration spans.
Worth noting for the record: for such a span, per-message finish_reason is now "tool_call" while the span-level gen_ai.response.finish_reasons attribute is still absent (:386-387 only sets it when span.finish_reason is not None). That asymmetry is pre-existing, but the inference makes it observable.
- 🟡 minor (question): The checkpointer limitation leaves the ticket's stated repro scope only partially fixed — is it tracked anywhere?
The code comment and PR description both acknowledge that with a LangGraph checkpointer the prefix never matches, so no reduction fires. HYBIM-988's repro section claims "any LangChain/LangGraph app using SplunkAOAsyncCallback where the agent makes at least one tool call", and thread persistence via a checkpointer is the standard production setup — so for a large share of affected users the — symptom (or worse, an earlier user turn rendered as the agent output) survives this fix. I don't see a follow-up ticket referenced.
One concrete alternative that covers both shapes without loosening the ToolNode protection: anchor on the last input message rather than requiring a whole-prefix match.
anchor = input_messages[-1] if input_messages else None
if full_history and anchor is not None and anchor in output_messages:
cut = len(output_messages) - 1 - output_messages[::-1].index(anchor)
remainder = output_messages[cut + 1 :]
if remainder:
output_messages = remainder[-1:]- Plain (no checkpointer) case: anchor is the last input message, remainder is exactly today's post-dedup remainder → identical result.
- Checkpointer case:
input={"messages":[user2]},output=[user1, ai1, user2, ai2]→ anchor found at index 2 →[ai2], which is the desired output (verified today's code emits all four messages here). - ToolNode parallel case: anchor (
ai_toolcall) is absent from[tool1, tool2]→ no trim, all messages survive. - Uses the last occurrence, so a repeated identical user turn still anchors correctly.
Happy for this to be a follow-up rather than a blocker, but please either adopt it or file the ticket so the gap doesn't get lost behind the "validated on lab0" sign-off.
Follow-ups
Suggested follow-up work that could be tracked as Jira tickets:
src/splunk_ao/converter/attribute_mapping.py:461-467: Pre-existing, not introduced here: when the output state is byte-identical to the input state the remainder is empty andgen_ai.output.messagesis serialized as"[]"(confirmed identical onorigin/main, and pinned bytest_orchestration_preserves_empty_full_history_suffix, tests/test_attribute_mapping.py:768). That is the same empty-output—in the UI this PR sets out to remove, just via a different path (a node that returns state unchanged). Worth deciding whether "no new messages" should omit the attribute entirely rather than emit an empty array, so consumers can distinguish "produced nothing" from "produced an empty output".src/splunk_ao/handlers/langchain/middleware.py:196-218:after_agent/aafter_agentalready reduce the root agent state to its last message at the handler layer, while_set_orchestration_contentnow performs the same reduction in the converter behind a prefix-match gate. Two layers implement overlapping policy with different trigger conditions, so the same application emits different output depending on which instrumentation path is active. Consolidating on one layer (converter-only, with the middleware passing state through) would remove the divergence. Raised as a follow-up in all three prior review runs and still open.src/splunk_ao/converter/attribute_mapping.py:217-239:_message_containeronly reportsfull_history=Truewhenmessagessits at the top level (container is source, line 228). Accumulated history arriving through a nestedupdatekey — the LangGraphCommandshape thatutils/serialization.pydoes produce, and which the new tests at lines 474-533 exercise for finish reasons — is classifiedfull_history=Falseand therefore never deduped or trimmed. Worth confirming whether a root agent output can arrive inCommandform; if so, HYBIM-988 reproduces there unchanged.
| history_stripped = False | ||
| if full_history and input_messages and output_messages[: len(input_messages)] == input_messages: | ||
| output_messages = output_messages[len(input_messages) :] | ||
| history_stripped = True | ||
| if history_stripped: | ||
| output_messages = output_messages[-1:] |
There was a problem hiding this comment.
🔵 nit (other): history_stripped is set and consumed with nothing in between, so the flag and the second if add indirection without buying anything — the trim can live in the branch that already established the precondition. (If you take the non-user-message suggestion below, fold it into this same branch.)
| history_stripped = False | |
| if full_history and input_messages and output_messages[: len(input_messages)] == input_messages: | |
| output_messages = output_messages[len(input_messages) :] | |
| history_stripped = True | |
| if history_stripped: | |
| output_messages = output_messages[-1:] | |
| if full_history and input_messages and output_messages[: len(input_messages)] == input_messages: | |
| output_messages = output_messages[len(input_messages) :][-1:] |
🤖 Generated by the Astra agent
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
🔵 nit (other): Three blank lines here — ruff format --check tests/test_attribute_mapping.py reports the file would be reformatted (ruff format --diff removes exactly this line). .pre-commit-config.yaml runs the ruff-format hook, so this fails pre-commit as-is.
🤖 Generated by the Astra agent
Co-authored-by: Fernando Correia <fercor@cisco.com>
Co-authored-by: Fernando Correia <fercor@cisco.com>
…d prefer last non-user message - Remove history_stripped flag; fold non-user-message trim into the single if block (r3824609327) - Replace [-1:] with last non-user-message search to avoid surfacing trailing user turns (r3824609186) - Rename test to reflect tool_call inference rather than unknown finish reason (r3824609453) - Remove extra blank line at line 688 (r3824609548) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Summary
Edge cases addressed:
Also bundled: `_with_finish_reasons` now infers `tool_call` finish reason when a message's parts contain a tool call but no explicit source finish reason is set. Explicit source finish reasons and tool-response messages are unaffected.
Known limitation (out of scope): With a LangGraph checkpointer, the root graph input is only the new turn while `on_chain_end` reports the full thread — prefix comparison fails and no reduction happens.
Test plan
Fixes https://splunk.atlassian.net/browse/HYBIM-988
Update (post-review, follow-up commit `58a1f79`):