Update openai and langgraph adapters to support events - #650
Conversation
cadbe49 to
d78590b
Compare
d78590b to
e5bbbf8
Compare
e5bbbf8 to
8c0767e
Compare
8c0767e to
7f061f4
Compare
amithad
left a comment
There was a problem hiding this comment.
Overall: a focused, high-quality PR 4 of the #523 stack. Both adapters match spec.md §10 as corrected in this PR, and the tests cover exactly the failure modes that would otherwise be silent (the empty-assistant-bubble regression, nested run_id pairing, the unserialisable-args path, the no-call_id item, and message_output_created staying ignored so messages are not doubled). CI is fully green (55 checks).
Spec documents (reviewed first): the spec.md and plan.md deltas are self-correcting and verifiable. §10 now separates correlation ids from boundary derivation and records that LangGraph, not only ADK, must derive boundaries; plan.md iteration 4 records every implementation-time decision with its reason, including the deviation from its own step 2. The claims I re-verified against the code all hold: started: set[str] is a local passed into _map_event (never on self), on_chat_model_start maps to nothing with MessageStart deferred to the first non-empty delta, OpenAI reads every id off the SDK's own events and remembers nothing, tool arguments are emitted whole, and _tool_arguments' broad except logs and leaves the call bracketed. One internal inconsistency introduced by the correction is inline (spec.md:794).
Spec conformance (dimension 6, against the PR 4 checklist): all implemented and tested. OpenAI stops filtering to ResponseTextDeltaEvent and maps tool_called/tool_output plus reasoning; LangGraph adds the on_chat_model_end/on_tool_start/on_tool_end branches with run_id correlation; both runner test suites moved from assert deltas == ["hi"] to event-sequence assertions while the framework_context round-trip assertions stay. The existing-test blast radius matches the spec's table exactly: only the two runner test files change, and the must-not-change list is untouched. The logger names follow the ak.<framework>.runner convention the other adapters use.
Findings: 1 suggestion and 1 question, both inline.
Summary-only:
- [suggestion] design.md has gone stale again, in a new spot this PR's learning created: its "Adapter boundary support" table says LangGraph's boundaries are "Explicit" via
on_chat_model_start/on_chat_model_endand the work is "handle more branches of anifit already has", but this PR established those events are unusable for message boundaries and added derivation with per-run memory. Fold this row into the design.md sync pass already requested on PR 649 (the comment on plan.md:210 there), so the sync fixes all four stale spots in one edit. - [suggestion] The PR body is again the unfilled template: no description and no issue link. Worth noting in the description that the "Breaking change" checkbox really belongs to PR 1's runner-contract change; what this PR itself changes observably is that OpenAI and LangGraph streams now carry real message boundaries, reasoning, and tool calls instead of the synthetic transitional pair.
No existing feedback on the PR, so nothing was skipped as a duplicate.
| - **LangGraph** has an id but its `on_chat_model_start` / `on_chat_model_end` fire around *every* | ||
| model call whether or not prose was streamed, so mapping them directly brackets a tool-calling | ||
| turn into a message with nothing in it — the empty assistant bubble §4 rule 4 exists to prevent. | ||
| It therefore ignores `on_chat_model_start` entirely, emits `MessageStart` on the first non-empty |
There was a problem hiding this comment.
[suggestion] This correction did not reach the §10 table above it, so the section now contradicts itself.
- The table row (spec.md:763) still instructs: "Add branches for
on_chat_model_start/on_chat_model_end/on_tool_start/on_tool_endto theif", while this bullet says the adapter "ignoreson_chat_model_startentirely" and the implementation has no such branch. - plan.md handles the same drift by annotating its step 2 inline ("The
on_chat_model_startbranch is gone, which deviates from step 2 above"); the table row deserves the same one-line touch, e.g. droppingon_chat_model_startfrom the list or adding "(start is derived, see below)". - Matters because
ak-dev-review-prextracts requirements from these tables: a reviewer of PR 5 or 6 walking §10 would flag the missing branch as an omission.
| :param item: The `RunItem` the event wraps. | ||
| :return: The AK events this item produces, empty when it maps to nothing. | ||
| """ | ||
| if name not in ("tool_called", "tool_output"): |
There was a problem hiding this comment.
[question] Handoff run items stay unmapped: deliberate non-goal, or a gap worth an event?
handoff_requested/handoff_occuredfall through here andtest_item_created_run_events_are_ignored_so_messages_are_not_doubledpins one of them as ignored, but neither spec.md §9/§10 nor plan.md records the decision, unlike every other ignore in this PR.- design.md's Motivation cites handoffs as information the old filter discards ("tool-call items and handoffs are dropped by the filter"), so a reader of the spec set would expect them to survive once the filter goes.
- The AK event model has a natural carrier:
StepStart/StepEnd(name= the target agent), which LangGraph declined for its own reasons (on_chain_*is too noisy) that do not apply here — a handoff is exactly the user-recognisable boundary that decision said steps should be. - If the answer is "non-goal for now": a line in spec.md §10 and in PR 7's fidelity matrix keeps the next reviewer from re-asking; if not, the mapping looks cheap while the raw items are already in hand.
Description
Type of Change
Related Issues
Fixes #
Relates to #
Changes Made
Testing
Checklist
Screenshots (if applicable)
Additional Notes